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

DEPS: Sync environment.yml with CI dep files #47287

Merged
merged 20 commits into from Jun 22, 2022

Conversation

mroeschke
Copy link
Member

@mroeschke mroeschke commented Jun 8, 2022

Also add a GHA job to check that the generated requirements-dev.txt file, synced from environment.yml, can be installed.

@mroeschke
Copy link
Member Author

@twoertwein any idea why adding some dependencies to environment.yml would uncover these typing errors?
https://github.com/pandas-dev/pandas/runs/6803850160?check_suite_focus=true

@twoertwein
Copy link
Member

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
Copy link
Member

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).

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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).

Copy link
Member

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.

@twoertwein
Copy link
Member

If pyqt has no type annotations (not sure whether that is the case), there is not much value in raising errors: could add # pyright: reportMissingImports = false at the top of pandas/io/clipboard/__init__.py

@twoertwein
Copy link
Member

If pyqt has no type annotations (not sure whether that is the case), there is not much value in raising errors: could add # pyright: reportMissingImports = false at the top of pandas/io/clipboard/__init__.py

I think pandas/io/clipboard/__init__.py is copied from Pyperclip: and already excluded from the "main" pyright run but not from the pyright reportGeneralTypeIssues run.

Would be good to add the excludes from

exclude = ["pandas/tests", "pandas/io/clipboard", "pandas/util/version"]
here something like

        # exclude tests
        "pandas/tests",
        # exclude vendored files
        "pandas/io/clipboard",
        "pandas/util/version",
        # and all files that currently don't pass
        ...

Happy to open a PR or you can include these changes in your PR.

@jreback jreback added the Dependencies Required and optional dependencies label Jun 10, 2022
@mroeschke mroeschke added this to the 1.5 milestone Jun 15, 2022
@mroeschke mroeschke added the CI Continuous Integration label Jun 15, 2022
Copy link
Member Author

@mroeschke mroeschke left a 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

@twoertwein
Copy link
Member

@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.

@mroeschke mroeschke modified the milestones: 1.5, 1.4.3 Jun 21, 2022
@jreback
Copy link
Contributor

jreback commented Jun 22, 2022

lgtm. (prob need to manually backport).

@jreback jreback merged commit fa5a604 into pandas-dev:main Jun 22, 2022
@jreback
Copy link
Contributor

jreback commented Jun 22, 2022

@meeseeksdev backport 1.4.x

@lumberbot-app

This comment was marked as resolved.

@lumberbot-app

This comment was marked as resolved.

@simonjayhawkins
Copy link
Member

There are typing failures on main from this commit.

@twoertwein
Copy link
Member

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).

@simonjayhawkins simonjayhawkins modified the milestones: 1.4.3, 1.4.4 Jun 24, 2022
yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
@simonjayhawkins simonjayhawkins modified the milestones: 1.4.4, 1.5 Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Dependencies Required and optional dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: dev requirements version inconsistencies CI: Incompatible versions in resolved dependencies
4 participants