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

Upgrade to tensorstore 0.1.72 and drop Python 3.9 #1277

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

normanrz
Copy link
Member

@normanrz normanrz commented Mar 25, 2025

Description:

  • Drops Python 3.9 support
  • Upgrades tensorstore to 0.1.72

Todos:

Make sure to delete unnecessary points or to check all before merging:

  • Updated Changelog
  • Updated Documentation

Sorry, something went wrong.

@normanrz normanrz self-assigned this Mar 26, 2025
@normanrz normanrz requested a review from markbader March 26, 2025 10:06
@@ -30,7 +30,7 @@ classifiers = [
"Programming Language :: Python :: 3.13",
]

requires-python = ">=3.9,<3.14"
requires-python = ">=3.10,<3.14"
Copy link
Member

Choose a reason for hiding this comment

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

Please also remove "Programming Language :: Python :: 3.9" in line 26

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 150-151 should be updated too.

@markbader
Copy link
Contributor

Looks good to me. Do we want to keep the python 3.9 support for cluster_tools? Otherwise we should update the pyproject.toml file there too.

@normanrz
Copy link
Member Author

I updated mypy and ruff and applied the changes. I realize this makes this PR unreviewable. Maybe you can have a look at just https://github.com/scalableminds/webknossos-libs/pull/1277/files/87f7a34e5bfc970ea51fdad8bc56509b809396be..4639d0ab881e15222afb16f9128483ecb644cf47

Copy link
Contributor

@markbader markbader left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Nice that we can finally use the fancy Union notation now.

@@ -399,7 +398,7 @@ def download(
*,
skip_volume_data: bool = False,
_return_context: bool = False,
) -> Union["Annotation", tuple["Annotation", ContextManager[None]]]:
) -> Union["Annotation", tuple["Annotation", AbstractContextManager[None]]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the Union[] notation not replaced here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ruff was too afraid to change the types with "strings" in it. I went through and changed them manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be problematic to use strings for the | operator. For python 3.11 and above, we could use type Self. https://stackoverflow.com/questions/33533148/how-do-i-type-hint-a-method-with-the-type-of-the-enclosing-class

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I reverted that. Ruff kept them for a good reason 😅

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.

None yet

3 participants