-
Notifications
You must be signed in to change notification settings - Fork 279
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
Future proofing: treat most warnings as errors in pytest, and fix existing ones #3380
Conversation
9fd63c8
to
2105651
Compare
45ae7dd
to
5b5e4b5
Compare
There remains only one failure in the py38_unittest job but I'm struggling to reproduce it locally. Any help would be greatly appreciated. |
f9cbb94
to
58954f3
Compare
@@ -47,7 +47,7 @@ install_requires = | |||
toml>=0.10.2 | |||
tqdm>=3.4.0 | |||
unyt>=2.8.0 | |||
python_requires = >=3.6 | |||
python_requires = >=3.6,<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.
To anyone wondering: the reasoning here is that the distutils builtin package is slanted for removal in Python 3.12 (which should be released around October 2023), but it is currently used in several places in yt, making it de facto incompatible with Python 3.12
Solving this doesn't seem like a priority but I'll document it as an issue !
58954f3
to
0216e48
Compare
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'd like to see one of the changes reworked, but otherwise this looks good to me.
# | ||
# >>> warnings from wrong values passed to numpy | ||
# these should normally be curated out of the test suite but they are too numerous | ||
# to deal with in a reasonable time at the moment. |
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 think some like this may arise from field detection.
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.
oh, good call ! I'll keep this in mind if I ever feel daring enough to fix these :)
pre-commit.ci autofix |
…ill make use of the distutils package, targetted for removal in Python 3.12)
…g can be done about it
Co-authored-by: Corentin Cadiou <contact@cphyc.me>
for more information, see https://pre-commit.ci
8dc5fd6
to
7a7eb8e
Compare
PR Summary
PR Summary
fix #3379
Though independent, this is complementary with #3376.
An important note is that there are some internal deprecation warnings with no obvious solution yet, so I'm adding them to the ignore-list in conftest.py but they will need to be addressed in future releases. Specifically, I'm talking about fields
("all", "px")
and("gas", "px")
(respectively"py"
,"dpx"
and"dpy"
), that are used internally in slice selectors, but never with explicit field/particle types. Getting to a stable solution here requires much more thought and design that what I'm solving here so I'll defer it to future works, and in any case it's not as urgent as other deprecation warnings because it's internal so it won't break haphazardly.TODO