-
Notifications
You must be signed in to change notification settings - Fork 444
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
[build] Upgrade py 3.10 #1770
[build] Upgrade py 3.10 #1770
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1770 +/- ##
==========================================
+ Coverage 96.58% 96.59% +0.01%
==========================================
Files 164 164
Lines 7895 7895
==========================================
+ Hits 7625 7626 +1
+ Misses 270 269 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@odulcy-mindee py3.9 end of life is Oct 25 but if already so many move to 3.10 i think we should follow that people will benefit from the improvements - Additional this would make it possible to move to python's built in types and away from the >= py3.11 deprecated |
@@ -24,7 +24,7 @@ jobs: | |||
uses: actions/setup-python@v5 | |||
with: | |||
# MacOS issue ref.: https://github.com/actions/setup-python/issues/855 & https://github.com/actions/setup-python/issues/865 | |||
python-version: ${{ matrix.os == 'macos-latest' && '3.11' || matrix.python }} | |||
python-version: ${{ matrix.os == 'macos-latest' && matrix.python == '3.10' && '3.11' || matrix.python }} |
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.
Why is there 3.11
written in this condition ?
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.
It's still the action bug with macOS for the python matrix == 3.10 uses python 3.11 to avoid the bug do you remember ? :)
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, but why matrix.python == '3.10' && '3.11' || matrix.python
? Shouldn't matrix.os == 'macos-latest' && matrix.python == '3.10'
be enough ?
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.
@odulcy-mindee syntax is a bit confusing the last && defines which value to use so
matrix.os == 'macos-latest' && matrix.python == '3.10' -> if both True use 3.11 (&& '3.11') else use the matrix.python version
We could also used the version before so if macos-latest use always python 3.11 the change is a bit more explicit (but can be fully removed later on if we change to py 3.11 & py 3.12)
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.
At the end with the current defined matrix.python array both commands does the same the change makes it only a bit more "fine grained" ^^
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.
Ah ok, thanks for the clarification
@odulcy-mindee you have to merge this (i can't ^^) and adjust the required test rules in the GitHub repo settings :) |
@felixdittrich92 done |
Thanks 😊 |
@@ -183,7 +183,7 @@ ignore_missing_imports = true | |||
[tool.ruff] | |||
exclude = [".git", "venv*", "build", "**/__init__.py"] |
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.
BTW, have **/__init__.py
to be excluded?
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.
Hey @kuraga :)
Yeah because it does strange things here:
init.py example:
from doctr.file_utils import is_tf_available, is_torch_available
if is_tf_available():
from .tensorflow import *
elif is_torch_available():
from .pytorch import * # type: ignore[assignment]
and if i remember it has raised circular import issues on other places 😅
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.
Thanks :)
This PR:
Move to minimum py 3.10 for next release
reason:
Fix mypy
minor numpy 2.0 compatibility fix int0 -> intp (does also exist in <2.0)