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

MAINT update lock files on CI #24363

Merged
merged 5 commits into from Sep 5, 2022

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Sep 5, 2022

I ran build_tools/update_environments_and_lock_files.py manually. It's been a while and a few packages needs an update.

One day we will have a bot to create such PRs and automatically merge them if nothing is broken:

@ogrisel
Copy link
Member Author

ogrisel commented Sep 5, 2022

It seems that the ordering of some of the conda-lock files has changed from alphabetical the previous ordering causing a bigger diff than needed.

I am using a freshly installed conda-lock from conda-forge on macos M1:

conda-lock                1.1.1              pyhd8ed1ab_0    conda-forge

For instance look at the line for freetype in the diff for the build_tools/azure/pylatest_conda_forge_mkl_osx-64_conda.lock file:

https://github.com/scikit-learn/scikit-learn/pull/24363/files#diff-2dd1533edec8d8601fe61b2a1b872013c909e9a97faec6eceae8d9a5999d5f84R42

@ogrisel
Copy link
Member Author

ogrisel commented Sep 5, 2022

The pip installed package URLs in conda-lock files also have a #sha256=... suffix which wasn't there before...

@ogrisel
Copy link
Member Author

ogrisel commented Sep 5, 2022

About the ordering problem, there was a bug reported by @lesteve a while ago but it was fixed for the past few releases of conda-lock:

conda/conda-lock#170

@ogrisel
Copy link
Member Author

ogrisel commented Sep 5, 2022

I am running an update from another computer to check if the diff is the same. Maybe it's just the topology of some dependencies that has changed.

EDIT: I get the same diff on a linux-x86_64 machine so the ordering change is probably expected / harmless or at least it is reproducible.

EDIT2: the #sha256=... suffixes have also been added in the conda-lock run I did on the linux-x86_64 machine. So I confirm that this seems stable.

@ogrisel
Copy link
Member Author

ogrisel commented Sep 5, 2022

The failure with pylatest_conda_mkl_no_openmp happens when pip tries to install the result of the successful build of scikit-learn dev:

Processing dependencies for scikit-learn==1.2.dev0
error: numpy 1.23.1 is installed but numpy<1.23.0,>=1.16.5 is required by {'scipy'}

So we have an inconsistent dependency set for build. The content of the generated environment.yml file is the following:

## build_tools/azure/pylatest_conda_mkl_no_openmp_environment.yml
# DO NOT EDIT: this file is generated from the specification found in the
# following script to centralize the configuration for CI builds:
# build_tools/update_environments_and_lock_files.py
channels:
  - defaults
dependencies:
  - python
  - numpy
  - blas[build=mkl]
  - scipy
  - cython
  - joblib
  - threadpoolctl
  - matplotlib
  - pandas
  - pyamg
  - pytest
  - pytest-xdist
  - pillow
  - codecov
  - pytest-cov
  - coverage=6.2
  - ccache

So it is probably a problem on the current state of the defaults channel of Anaconda.org: it's using scipy 1.7 with numpy 1.23 while scipy 1.9 is available on other distribution channels (pypi and conda-forge namely).

@ogrisel
Copy link
Member Author

ogrisel commented Sep 5, 2022

I undid the update of build_tools/azure/pylatest_conda_mkl_no_openmp_osx-64_conda.lock to wait for the Anaconda.org defaults channel to update their scipy version to get a consistent set of dependencies on that distribution.

I also tagged the last commit message to trigger all the ancillary CI jobs.

@lesteve
Copy link
Member

lesteve commented Sep 5, 2022

We were using conda-lock 1.0.5 (rather than 1.1.1), for example see sklearn/_min_dependencies.py. That could be the reason of the ordering change.

@ogrisel
Copy link
Member Author

ogrisel commented Sep 5, 2022

We were using conda-lock 1.0.5 (rather than 1.1.1), for example see sklearn/_min_dependencies.py. That could be the reason of the ordering change.

Shall I update sklearn/_min_dependencies.py to point to 1.1.1 or is there a good reason to pin conda-lock to 1.0.5?

@lesteve
Copy link
Member

lesteve commented Sep 5, 2022

Shall I update sklearn/_min_dependencies.py to point to 1.1.1 or is there a good reason to pin conda-lock to 1.0.5?

1.0.5 was the latest version at the time, I think it is fine using 1.1.1 if that works and updating sklearn/_min_dependencies.py.

@ogrisel
Copy link
Member Author

ogrisel commented Sep 5, 2022

The pypy failure is caused by an upgrade from pypy37 to pypy39 that triggers a bug in the vendored cloupickle of joblib.

This broken import was fixed in cloudpipe/cloudpickle#469 but we first need to release a new joblib version with an updated cloudpickle.

In the mean time, let me revert the update of the pypy build in this PR.

@lesteve
Copy link
Member

lesteve commented Sep 5, 2022

About the pylatest_conda_mkl_no_openmp_osx-64_conda failure (scipy 1.7 and numpy 1.23 incompatibility), it seems like scipy 1.7.3 is meant to be compatible with numpy 1.23 e.g. https://docs.scipy.org/doc/scipy/dev/toolchain.html#numpy.

I opened scipy/scipy#16964 to discuss this.

Copy link
Member

@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

Quickly browsing the diff, LGTM!

Full disclosure: I haven't tried to run the script locally to see whether I see a consistent ordering.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants