-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
DEPS: Sync environment.yml with CI dep files #47287
Conversation
@twoertwein any idea why adding some dependencies to |
The reportMissingImports from pyright indicate that the PyQt package is no longer installed. But many of the mypy errors look not related to PyQt: is it using a newer version of numpy (locally I'm using numpy 1.22.4)? |
@@ -31,8 +31,7 @@ dependencies: | |||
- jinja2 | |||
- lxml | |||
- matplotlib | |||
# TODO: uncomment after numba supports py310 | |||
#- numba | |||
- numba |
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.
This could cause installing an older numpy version which could(?) explain most of the errors (but not the pyqt stuff).
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.
As there is one dedicated CI run for numpy-dev, it would make sense to use the latest numpy compatible with numba (even for typing). Reverting #45244 would probably fix most of the numpy errors.
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.
This file should only affect our specific PY 3.10 build which just runs the unit tests though. The typing checks should have an environment that is set up by environment.yml
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.
Sorry, I meant that since we anyways run the unit tests with NumPy-dev in a separate workflow, prioritizing the latest numba version over the latest (released) NumPy version (in environment.yml
) could be fine. Either way, it would be good to limit the numba version or the numpy version in environment.yml
.
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.
As there is one dedicated CI run for numpy-dev, it would make sense to use the latest numpy compatible with numba (even for typing). Reverting #45244 would probably fix most of the numpy errors.
The problem I have with this is that new contribritors when setting up an environment will get the latest numpy and have mypy errors by default.
We should make the contributor experience pain free so (imo) we should use environment.yaml for the typing validation to match the local dev env .
Otherwise, this just makes it difficult for people to contribute to the typing issues.
Now, numba is included in environment.yaml so I'm not sure why when I set up a clean dev locally I get numpy 1.23.1 and on ci we get 1.22.4 (maybe there is some caching on ci?)
My comments here are from looking into this a couple of weeks ago. So this comment here now maybe out of date. Will look again soon.
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.
Now, numba is included in environment.yaml so I'm not sure why when I set up a clean dev locally I get numpy 1.23.1 and on ci we get 1.22.4 (maybe there is some caching on ci?)
I must admit that I don't use the official way to setup a pandas-dev env, but it would be great to ensure that the officially documented pandas-env does not cause mypy errors.
Maybe numba has different numpy-constraints on conda-forge (or conda installs incompatible versions)? When I ask poetry to install numba = ">=0.53.1"
(as in environment.yml) and numpy = ">=1.23.0"
, it is unable to find a solution (at least not on Linux with 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.
I must admit that I don't use the official way to setup a pandas-dev env, but it would be great to ensure that the officially documented pandas-env does not cause mypy errors.
yes I need to double check that's still true.
If pyqt has no type annotations (not sure whether that is the case), there is not much value in raising errors: could add |
I think Would be good to add the excludes from Line 160 in 297c59a
Happy to open a PR or you can include these changes in your PR. |
e24984e
to
9461d8d
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.
@twoertwein would appreciate another look at the tying changes
The ignore comments look good to me (just a bit annoying that we need to undo those changes when numba supports numpy 1.22.4). It sounds reasonable to me to use the latest numba-compatible numpy and numpy-dev for testing but technically, there could be some weird behavior with the newest numpy that is already fixed in numpy-dev. |
lgtm. (prob need to manually backport). |
@meeseeksdev backport 1.4.x |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There are typing failures on main from this commit. |
It seems that the CI installed numpy 1.22.4 and numba 0.55.2. For some reason, they did not happily co-exist before (even though neither of them had a new release recently). |
Also add a GHA job to check that the generated
requirements-dev.txt
file, synced fromenvironment.yml
, can be installed.