Remove Python 2.7 support and six.py #11418

sepeth · 2024-05-22T14:40:20Z

Hello,

This is my first contribution to FoundationDB. I thought I could start from something simple ^-^

This change removes Python 2.7 support and six.py (resolves #11328 and #11329). Most of it is straightforward, such as six.text_type -> str, six.integer_types -> int. OTOH, decisions I made:

Kept int2byte as it is nicer to read than struct.Struct(">B").pack, and it was used many times.
As for the tester name, I decided to keep the name "python", but the executable it points to is still "python3". Every other language on the list was without its version, so it made sense this way.
Deleted most mentions of Python 2.7 as it has been dead for a long time.

I first tested with the class scheduling examples in the tutorial and validated that fdb.__path__ was pointing to the fdb that I modified and installed.

Then, I tested via run_tester_loop.sh & bindingtester.py. This took a bit of time and required some fiddling with sources. But eventually, I got it running, and all python tests have passed. A few issues I had to overcome:

bindingtester.py is prepending a path to sys.path, which didn't allow me to run run_tester_loop.sh from the source dir.
bindingtester weren't copied/built into the build tree (due to a check in bindingtester/CMakeLists.txt). I am not sure about the implications of enabling this, but it would be nice if bindingtester is easier to run in case changes made to bindings. I didn't want to make assumptions and changes about this, and wanted to keep this solely for removing Python 2.7 support, but I am happy to look into this in another PR (and happy to hear your suggestions).

Code-Reviewer Section

The general pull request guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

The PR has a description, explaining both the problem and the solution.
The description mentions which forms of testing were done and the testing seems reasonable.
Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

xis19

Thank you very much for your contribution!!

xis19 · 2024-05-22T18:36:47Z

bindings/python/fdb/directory_impl.py

@@ -613,17 +612,17 @@ def _is_prefix_empty(self, tr, prefix):

 def _to_unicode_path(path):
    if isinstance(path, bytes):
-        path = six.text_type(path)
+        path = str(path)


I would guess this may not work as bytes are not encoded in Python while str is encoded by utf-8 by default. I think it might be better to do path.decode() to introduce a conversion.

Hmm, happy to change it, but wouldn't they be the same? Both str(path) or path.decode() would return a unicode string in Python 3, no? 🤔

Well, I think they are somewhat different, I just tried it on Python 3.12

Python 3.12.0 (main, Nov 9 2023, 17:03:26) [Clang 15.0.0 (clang-1500.0.40.1)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> s=b"1234" >>> print(s) b'1234' >>> type(s) <class 'bytes'> >>> t=str(s) >>> print(t) b'1234' >>> type(t) <class 'str'> >>> print(f"{t}") b'1234' >>> print(f"[{t}]") [b'1234'] >>>

Oh I see, you are absolutely right. Continuing on your answer,

>>> s.decode() == str(s) False

But for str to decode bytes, encoding must be passed:

>>> s.decode() == str(s, 'utf-8') True

Also validated with the docs:

If neither encoding nor errors is given, str(object) returns type(object).__str__(object), which is the “informal” or nicely printable string representation of object. For string objects, this is the string itself. If object does not have a __str__() method, then str() falls back to returning repr(object).

https://docs.python.org/3/library/stdtypes.html#str

TIL, and I will make the change.

May you please also have a look at the comment for _to_unicode_path?

Yeah, Python has plenty of pitfalls but better than C++, while I would suggest we move to Rust sometime since things would be more explicit -- in Rust str must be encoded and bytes are simply [u8]; so you can never simply [u8].to_str() without checking the encoding.

Yeah ^-^ Also TIL, python -b to catch this. Apparently, it is a thing, and there's also python -bb to treat these as errors.

bindings/python/fdb/directory_impl.py

xis19 · 2024-05-23T05:23:29Z

Let's check for the binding correctness in the CI after you changed the code. We have plenty of tests to make sure we do not break things, but still, not enough.

foundationdb-ci · 2024-05-23T21:25:15Z

Result of foundationdb-pr-clang-ide on Linux CentOS 7

Commit ID: dc5510a
Duration 0:22:28
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-05-23T21:56:41Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: dc5510a
Duration 0:53:54
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-05-23T22:01:58Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: dc5510a
Duration 0:59:19
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

sepeth · 2024-05-28T14:22:27Z

Hi there, please let me know if there's anything wrong. I see some tests are failing but couldn't be sure if they are related - I see they fail in some other PRs as well.

jzhou77 · 2024-05-28T16:56:54Z

Hi there, please let me know if there's anything wrong. I see some tests are failing but couldn't be sure if they are related - I see they fail in some other PRs as well.

python3 -m joshua.joshua list --stopped failure doesn't seem to be related to the Python changes in this PR (maybe merge with HEAD can resolve some of the failures fixed recently).

I saw some changes for bindingtester, so @xis19 can you make sure bindingtester can still pass joshua test?

foundationdb-ci · 2024-07-24T21:50:16Z

Result of foundationdb-pr-clang-ide on Linux CentOS 7

Commit ID: dc5510a
Duration 0:21:41
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-07-24T22:03:05Z

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Commit ID: dc5510a
Duration 0:34:33
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-07-24T22:06:19Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: dc5510a
Duration 0:37:46
Result: ❌ FAILED
Error: Error while executing command: docker build --label "org.foundationdb.version=${FDB_VERSION}" --label "org.foundationdb.build_date=${BUILD_DATE}" --label "org.foundationdb.commit=${COMMIT_SHA}" --progress plain --build-arg FDB_VERSION="${FDB_VERSION}" --build-arg FDB_LIBRARY_VERSIONS="${FDB_VERSION}" --build-arg FDB_WEBSITE="${FDB_WEBSITE}" --tag foundationdb/base:${FDB_VERSION}-${COMMIT_SHA} --file Dockerfile --target base .. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci · 2024-07-24T22:06:31Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: dc5510a
Duration 0:37:54
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-07-24T22:14:20Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: dc5510a
Duration 0:45:44
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-07-24T22:15:13Z

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: dc5510a
Duration 0:46:39
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-07-24T22:21:07Z

Result of foundationdb-pr-clang-arm on Linux CentOS 7

Commit ID: dc5510a
Duration 0:52:32
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

sepeth · 2024-09-20T14:57:37Z

Hi @xis19 and @spraza ICYMI.

Currently python bindings with asyncio is not working (they are from python 3.4 or below I think), I would like to look into it, but I think this PR makes that easier.

spraza · 2024-09-20T17:24:32Z

Hi @xis19 and @spraza ICYMI.

Currently python bindings with asyncio is not working (they are from python 3.4 or below I think), I would like to look into it, but I think this PR makes that easier.

Thanks for the reminder, @sepeth. I don't have enough context on this PR, I'll see if @xis19 can review this otherwise I can find some cycles to review.

foundationdb-ci · 2024-10-02T17:41:40Z

Result of foundationdb-pr-clang-ide on Linux CentOS 7

Commit ID: 4cf353c
Duration 0:21:44
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-10-02T18:03:41Z

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

Commit ID: 4cf353c
Duration 0:43:44
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-10-02T18:09:18Z

Result of foundationdb-pr-macos on macOS Ventura 13.x

Commit ID: 4cf353c
Duration 0:49:20
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-10-02T18:10:13Z

Result of foundationdb-pr-clang-arm on Linux CentOS 7

Commit ID: 4cf353c
Duration 0:50:15
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-10-02T18:16:59Z

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

Commit ID: 4cf353c
Duration 0:57:02
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)
Cluster Test Logs zip file of the test logs (available for 30 days)

foundationdb-ci · 2024-10-02T18:22:43Z

Result of foundationdb-pr-clang on Linux CentOS 7

Commit ID: 4cf353c
Duration 1:02:45
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2024-10-02T18:27:00Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 4cf353c
Duration 1:06:56
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

xis19

LGTM

jzhou77 requested a review from xis19 May 22, 2024 15:41

jzhou77 closed this May 22, 2024

jzhou77 reopened this May 22, 2024

This comment was marked as outdated.

Sign in to view

xis19 requested changes May 22, 2024

View reviewed changes

sepeth force-pushed the remove-python-2.7-support branch from 4485534 to dc5510a Compare May 23, 2024 06:00

sepeth mentioned this pull request May 23, 2024

Use pathlib.Path in Directory layer in Python bindings if possible #11427

Open

sepeth requested a review from xis19 May 23, 2024 15:56

jzhou77 closed this Jul 24, 2024

jzhou77 reopened this Jul 24, 2024

Remove Python 2.7 support and six.py

4cf353c

sepeth force-pushed the remove-python-2.7-support branch from dc5510a to 4cf353c Compare September 19, 2024 13:45

xis19 closed this Oct 2, 2024

xis19 reopened this Oct 2, 2024

xis19 approved these changes Oct 2, 2024

View reviewed changes

jzhou77 merged commit 39172d7 into apple:main Oct 3, 2024
7 checks passed

Remove Python 2.7 support and six.py #11418

Remove Python 2.7 support and six.py #11418

Conversation

sepeth commented May 22, 2024 • edited Loading

Code-Reviewer Section

For Release-Branches

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

xis19 left a comment

Choose a reason for hiding this comment

xis19 May 22, 2024

Choose a reason for hiding this comment

sepeth May 22, 2024

Choose a reason for hiding this comment

xis19 May 22, 2024

Choose a reason for hiding this comment

sepeth May 22, 2024 • edited Loading

Choose a reason for hiding this comment

xis19 May 23, 2024

Choose a reason for hiding this comment

sepeth May 23, 2024

Choose a reason for hiding this comment

xis19 commented May 23, 2024

foundationdb-ci commented May 23, 2024

Result of foundationdb-pr-clang-ide on Linux CentOS 7

foundationdb-ci commented May 23, 2024

Result of foundationdb-pr-clang on Linux CentOS 7

foundationdb-ci commented May 23, 2024

Result of foundationdb-pr on Linux CentOS 7

sepeth commented May 28, 2024 • edited Loading

jzhou77 commented May 28, 2024

foundationdb-ci commented Jul 24, 2024

Result of foundationdb-pr-clang-ide on Linux CentOS 7

foundationdb-ci commented Jul 24, 2024

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

foundationdb-ci commented Jul 24, 2024

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

foundationdb-ci commented Jul 24, 2024

Result of foundationdb-pr-clang on Linux CentOS 7

foundationdb-ci commented Jul 24, 2024

Result of foundationdb-pr on Linux CentOS 7

foundationdb-ci commented Jul 24, 2024

Result of foundationdb-pr-macos on macOS Ventura 13.x

foundationdb-ci commented Jul 24, 2024

Result of foundationdb-pr-clang-arm on Linux CentOS 7

sepeth commented Sep 20, 2024

spraza commented Sep 20, 2024 • edited Loading

foundationdb-ci commented Oct 2, 2024

Result of foundationdb-pr-clang-ide on Linux CentOS 7

foundationdb-ci commented Oct 2, 2024

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

foundationdb-ci commented Oct 2, 2024

Result of foundationdb-pr-macos on macOS Ventura 13.x

foundationdb-ci commented Oct 2, 2024

Result of foundationdb-pr-clang-arm on Linux CentOS 7

foundationdb-ci commented Oct 2, 2024

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

foundationdb-ci commented Oct 2, 2024

Result of foundationdb-pr-clang on Linux CentOS 7

foundationdb-ci commented Oct 2, 2024

Result of foundationdb-pr on Linux CentOS 7

xis19 left a comment

Choose a reason for hiding this comment

sepeth commented May 22, 2024 •

edited

Loading

sepeth May 22, 2024 •

edited

Loading

sepeth commented May 28, 2024 •

edited

Loading

spraza commented Sep 20, 2024 •

edited

Loading