Skip to content
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

[python] Python 3.12 support #3001

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[python] Python 3.12 support #3001

wants to merge 4 commits into from

Conversation

johnkerl
Copy link
Member

@johnkerl johnkerl commented Sep 16, 2024

For issue #1849 -- which has a long history.

See also #2999 and [sc-53002].

This needs single-cell-data/SOMA#222

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.65%. Comparing base (928c281) to head (b7db9ed).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3001      +/-   ##
==========================================
+ Coverage   83.54%   83.65%   +0.11%     
==========================================
  Files          51       51              
  Lines        5434     5434              
==========================================
+ Hits         4540     4546       +6     
+ Misses        894      888       -6     
Flag Coverage Δ
python 83.65% <ø> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api 83.65% <ø> (+0.11%) ⬆️
libtiledbsoma ∅ <ø> (∅)

Copy link
Contributor

@thetorpedodog thetorpedodog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now leaving comments as I go through each commit. apologies if this is already resolved in a later one

Comment on lines 120 to 124
git clone [email protected]:single-cell-data/SOMA.git soma
cd ./SOMA
git checkout kerl/python-3.12
pip install ./
python -c 'import somacore; print(somacore.__version__)'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pip install git+https://github.com/single-cell-data/SOMA.git@kerl/python-3.12 might be able to replace this with a one-liner that leaves your venv in a better state?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thetorpedodog thanks! This stanza was my copy-paste of another time someone wanted to run tiledbsoma CI against a non-merged somacore mod ... I'll save off your advice in my cheat-sheet for next time -- thank you!

@thetorpedodog
Copy link
Contributor

From the build log:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/__w/TileDB-SOMA/TileDB-SOMA/venv-soma/lib/python3.10/site-packages/tiledbsoma/__init__.py", line 103, in <module>
    from ._collection import Collection
  File "/__w/TileDB-SOMA/TileDB-SOMA/venv-soma/lib/python3.10/site-packages/tiledbsoma/_collection.py", line 3[7](https://github.com/single-cell-data/TileDB-SOMA/actions/runs/10895777089/job/30234556256#step:18:8), in <module>
    from ._dataframe import DataFrame
  File "/__w/TileDB-SOMA/TileDB-SOMA/venv-soma/lib/python3.10/site-packages/tiledbsoma/_dataframe.py", line 17, in <module>
    from tiledbsoma import _new_shape_feature_flag_enabled
ImportError: cannot import name '_new_shape_feature_flag_enabled' from partially initialized module 'tiledbsoma' (most likely due to a circular import) (/__w/TileDB-SOMA/TileDB-SOMA/venv-soma/lib/python3.[10](https://github.com/single-cell-data/TileDB-SOMA/actions/runs/10895777089/job/30234556256#step:18:11)/site-packages/tiledbsoma/__init__.py)

This is because at the point this line is executed, the symbol _new_shape_feature_flag_enabled is not yet available. The line

_new_shape_feature_flag = os.getenv("SOMA_PY_NEW_SHAPE") is not None

needs to be moved above all the imports. (Alternately, it might make sense to create a tiledbsoma._flags module so that there’s no circular dependency between submodules and the tiledbsoma/__init__.py file.

@johnkerl
Copy link
Member Author

johnkerl commented Sep 17, 2024

needs to be moved above all the imports

@thetorpedodog thank you!!! I got some 'new style lint' here -- I don't know why, I didn't change any linter versions -- and was forced to move imports :(

There's something weird to debug here -- I'll see if I can make everything in the CI toolchain happy simultaneously -- thank you!!!

Also I'll try the _flags module idea -- this sounds promising -- thank you! :)

@johnkerl
Copy link
Member Author

@thetorpedodog

And re

There's something weird to debug here -- I'll see if I can make everything in the CI toolchain happy simultaneously -- thank you!!!

I have traced that to here:

https://github.com/single-cell-data/TileDB-SOMA/pull/3004/files#r1763424424

@johnkerl johnkerl force-pushed the kerl/python-3.12-support branch 2 times, most recently from 9790ab0 to db17d38 Compare September 17, 2024 20:03
@iosonofabio
Copy link

Hi guys, thanks for your amazing package.

I understand this is in flux as we write - as a power user I would kindly like to ask if there's anything I can do to help push this through - I really need it and workarounds are a decent pain.

(As I understand, now there is a buggy typechecker stalling CI stalling 3.12 adoption, which itself was released 12 months ago. If inaccurate feel free to correct)

@johnkerl
Copy link
Member Author

@iosonofabio thank you! Your assessment is correct -- current status is here: #1849 (comment) and @ryan-williams will be moving this forward

@ryan-williams
Copy link
Member

I can't add you as a reviewer since you opened this PR, @johnkerl, but I would like your review on the latest changes that I've pushed here 🙏

Depending on typeguard@HEAD may trip up SOMA developers who have existing typeguard 4.2.1 or 4.3.0 installations, but I'm not sure what the other option is. As soon as agronholm/typeguard#490 makes it into a release, we can move to that. Could also try to only depend on typeguard@HEAD in Python 3.12, but I thought that was more complex than it was worth.

Copy link
Member Author

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!! :)

@@ -783,7 +783,9 @@ def make_multiply_indexed_dataframe(
"domain": [[-1000, 1000]],
"coords": [{"bogus": True}],
"A": None,
"throws": TypeError,
# Disable Typeguard while asserting this error, otherwise a typeguard.TypeCheckError is
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hvae a GitHub issue tracking the 'real' typeguard release, and let's put its URL here as a to-do comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't due to using Typeguard@HEAD; it will remain relevant when we move back to a proper Typeguard release.

The reason for it is: in 3.12, better type-checking was causing a TypeCheckError to occur before the TypeError we were verifying here.

I worked around it by disabling type-checking while triggering this error, so we can continue to verify what we were previously.

throws = io.get("throws", None)
if throws:
if isinstance(throws, tuple) and not throws[1]:
# Disable Typeguard, verify actual runtime error type (avoid
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto the above to-do URL/comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, todo related to upgrading Typeguard is not applicable here.

@johnkerl
Copy link
Member Author

Depending on typeguard@HEAD may trip up SOMA developers who have existing typeguard 4.2.1 or 4.3.0 installations, but I'm not sure what the other option is

Agreed. And it only affects folks on 3.12, so any dev who really wants to avoid this can use 3.11.

@@ -783,7 +783,9 @@ def make_multiply_indexed_dataframe(
"domain": [[-1000, 1000]],
"coords": [{"bogus": True}],
"A": None,
"throws": TypeError,
# Disable Typeguard while asserting this error, otherwise a typeguard.TypeCheckError is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't due to using Typeguard@HEAD; it will remain relevant when we move back to a proper Typeguard release.

The reason for it is: in 3.12, better type-checking was causing a TypeCheckError to occur before the TypeError we were verifying here.

I worked around it by disabling type-checking while triggering this error, so we can continue to verify what we were previously.

throws = io.get("throws", None)
if throws:
if isinstance(throws, tuple) and not throws[1]:
# Disable Typeguard, verify actual runtime error type (avoid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, todo related to upgrading Typeguard is not applicable here.

typeguard==4.2.1
# Python 3.12 support requires https://github.com/agronholm/typeguard/pull/490.
# Use Typeguard @ HEAD until that PR is included in a release.
typeguard @ git+https://github.com/agronholm/typeguard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be the place for a TODO / issue link related to upgrading Typeguard. Do you want me to change this comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #3216 and referenced it here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants