- 
                Notifications
    You must be signed in to change notification settings 
- Fork 140
Drop Python 3.8, add 3.13 #694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
c761cff    to
    48c5bd5      
    Compare
  
    a200bb5    to
    c7326f4      
    Compare
  
    794c62b    to
    bf725ec      
    Compare
  
    0db2839    to
    fd57843      
    Compare
  
    | This is now ready for review. But we may not merge until early next due to holiday period availability. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM.
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.12" | ||
| python-version: "3.13" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to confirm, the -cp38- part of the wheel name in PyPi means it is compatible with 3.8+, but in fact the wheel was built using 3.12 (and will now be built with 3.13 and will contain a -cp39- name fragment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tell cibuildwheel to build for these minimums: cp39-win_amd64, cp39-manylinux_x86_64, cp39-manylinux_aarch64, cp39-macosx_x86_64, and cp39-macosx_arm64. And we tell PyO3 to compile with ABI compat to abi3-py39. It is those two tools that make sure during the build-binary step to compile for 3.9 minimum. cibuildwheel sometimes runs inside a container, and it's a certain container they build within to get older libc compat and such (ref https://github.com/pypa/manylinux for Linux for instance). May have to reference those projects for more details or look at the logs of the builds at https://github.com/temporalio/sdk-python/actions/workflows/build-binaries.yml.
| TEMPORAL_TEST_PROTO3: 1 | ||
| run: | | ||
| poetry add "protobuf<4" | ||
| poetry add --python 3.9 "protobuf<4" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a comment explaining the --python 3.9 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit I do not remember exactly, but I think it was saying there wasn't compatibility with some of the other dependencies
| ``True`` and this matcher is on ``children`` of a parent. | ||
| """ | ||
|  | ||
| exclude: Set[str] = frozenset() # type: ignore | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can now use the usual lower-case type names for collection types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, though inconsequential here I think. But would be opposed to a general porting.
| else: | ||
| _trace("__init__ unrecognized with args %s", args) | ||
|  | ||
| def __getattribute__(self, __name: str) -> Any: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit on existing code: double-underscore prefixes in Python should be (rarely) used, only when name-mangling is explicitly required. I don't think it's ever appropriate for a local variable in a function or a parameter name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I think at the time I wrote this years ago, typeshed defined this signature as __name at https://github.com/python/typeshed/blob/2116158891a3c88b97038617855ce216499833f9/stdlib/builtins.pyi#L115 and I think I needed to match it. It was only a year ago that they updated it: python/typeshed#11250.
| def skip_unfinished_handler_tests_in_older_python(): | ||
| # These tests reliably fail or timeout in 3.9 | ||
| if sys.version_info < (3, 10): | ||
| pytest.skip("Skipping unfinished handler tests in Python < 3.10") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I still haven't pinned this down. I think it's mostly or always under time-skipping? This PR shows that in fact they do not seem to reliably fail any longer: #724 Basically I was trying to study the problem and of course they stopped failing.
| Hi, I think you should also raise the version in the configuration for ruff https://github.com/temporalio/sdk-python/blob/main/pyproject.toml#L216. | 
| Thanks! I have opened an issue at #728 | 
What was changed
Drop Python 3.8 support and confirm Python 3.13 support
Specifically this included:
featuresrepo namedpython-version-upgradeto pass CIcibuildwheeland aligngrpcioand `grpcio-toolsprotocinstall step to wheel building since it was removed as part of pyproject.toml: Removed protoc-wheel dependency #684 and we didn't notice until now it broke wheel buildingfix-wheel.pyscript to unzip wheel to fix it instead ofwheel unpackwhich was failing on macos for strange hash mismatchos.statfrom sandbox restrictions since as of gh-106922: Support multi-line error locations in traceback (attempt 2) python/cpython#112097 it appears that is invoked even when displaying errorsAfter the PR is approved, I will change the required passing CI steps to be 3.9/3.13.
Checklist