CI: move Linux and MacOS Azure builds to conda lock files #22448

lesteve · 2022-02-11T10:09:17Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

This is a start:

I replaced the two pylatest_conda_forge_mkl builds (Linux and MacOS) with two lock files
I added an environment.yml used for these two builds
I added a script to generate these two lock files.

Any other comments

Right now I am thinking that the bot functionality is decoupled from using lock files in all the builds and it could happen in a second step.

thomasjpfan · 2022-02-11T14:39:47Z

Note that the lock file in the development version of conda-lock is a yaml file which I think is easier to parse if we need to.

lesteve · 2022-02-11T15:08:23Z

OK I think I saw some discussions about this but did not investigate further. Do you think we should wait for a conda-lock release then?

As long as the diff between two lock files (with the same conda lock command but after one dependency has a new release) is reasonably readable (full disclosure: I have not checked maybe the order can change quite a bit).

lesteve · 2022-02-11T15:10:21Z

One of the nice thing of the conda-lock development release is the support for pip section in environment.yml with poetry (we may need this for some builds where we combine conda + pip, e.g. the one using the conda defaults channel, not 100% sure)

thomasjpfan · 2022-02-13T13:34:17Z

One of the nice thing of the conda-lock development release is the support for pip section in environment.yml with poetry

There are few nice features in the dev version we can use. I would wait for their release.

ogrisel · 2022-02-15T19:43:34Z

There are few nice features in the dev version we can use. I would wait for their release.

Can we install the dev version (e.g. a fixed commit hash) with a pip git URL in the mean time?

lesteve · 2022-02-16T13:30:00Z

I am seeing weird errors with the conda-lock development version, I opened conda/conda-lock#148 for one of them.

I think I can keep going moving more builds to environment.yml files with conda-lock 0.13.2 in this PR, see if there are any show-stopper, and then reassess the situation with the conda-lock dev version (or the new release if there has been any).

lesteve · 2022-02-16T14:12:05Z

As an alternative I could do a first PR using environment.yml in all the builds and not using lock files. Once we have everything in environment.yml, moving to lock files would me a bit more straightforward.

Also regenerate lock files

lesteve · 2022-02-16T18:31:22Z

As long as the diff between two lock files (with the same conda lock command but after one dependency has a new release) is reasonably readable (full disclosure: I have not checked maybe the order can change quite a bit).

About the diff between lock files, you can see in d2a6dea#diff-708171dcd71ffc9f7ed79f676e2cb3dd1b923ba3e21319eb5e91613f12737120 that the diff is reasonably readable even if the order changes a bit, for example requests is not at the same place.

ogrisel · 2022-02-18T17:52:26Z

I am seeing weird errors with the conda-lock development version, I opened conda/conda-lock#148 for one of them.

I think I can keep going moving more builds to environment.yml files with conda-lock 0.13.2 in this PR, see if there are any show-stopper, and then reassess the situation with the conda-lock dev version (or the new release if there has been any).

Since the bug has been fixed in the dev branch of conda-lock we can consider using the dev version again (installed from a fixed git hash for stability), in order to use the new features if needed.

…happier

thomasjpfan

With the update of the Python version, the build_tools/azure/update_environments_and_lock_files.py now runs on my M1 machine.

There are likely merge conflicts from: #23174

…nto ci-lock-file

lesteve · 2022-05-10T08:19:54Z

In my last commit, CI is green for all the nightly builds (including the pip-nogil build).

Right now, the python-nogil lockfile is generated manually rather than with the update_environments_and_lock_files.py script. I added a comment at the top of the lockfile with the docker command I use. In an ideal world, the update_environments_and_lock_files.py would do it but maybe this can be done in a further PR?

ogrisel · 2022-05-10T09:53:13Z

In an ideal world, the update_environments_and_lock_files.py would do it but maybe this can be done in a further PR?

+1 for keeping it manual for the nogil build for now.

thomasjpfan

I'm okay with the handling of nogil. I tested it on m1 and it works as expected.

thomasjpfan · 2022-05-10T17:18:10Z

azure-pipelines.yml

        DOCKER_CONTAINER: 'i386/debian:11.2'
-        JOBLIB_VERSION: 'min'
+        DISTRIB: 'debian-32'
+        LOCK_FILE: './build_tools/azure/debian_atlas_32bit_lock.txt'
        # disable pytest xdist due to unknown bug with 32-bit container
        PYTEST_XDIST_VERSION: 'none'


Can we remove PYTEST_XDIST_VERSION as well? I do not think it is being used anymore.

PYTEST_XDIST_VERSION is still used in the test script:

scikit-learn/build_tools/azure/test_script.sh

Lines 65 to 66 in 7bdd426

if [[ "$PYTEST_XDIST_VERSION" != "none" ]]; then

TEST_CMD="$TEST_CMD -n$CPU_COUNT"

Longer term, I was thinking about cleaning up environment variables like PYTEST_XDIST_VERSION and COVERAGE and check whether a package is installed instead of relying on a environment variable.

thomasjpfan · 2022-05-11T18:29:40Z

build_tools/azure/python_nogil_lock.txt

+# The content below has been generated manually with the following command:
+# docker run -v $PWD:/scikit-learn -it nogil/python bash -c 'pip install pip-tools; pip-compile /scikit-learn/build_tools/azure/python_nogil_requirements.txt -o /scikit-learn/build_tools/azure/python_nogil_lock.txt'
+#
+# The reason behind it is that you need python-nogil to generate the pip lock
+# file. Using pip-compile --index and --extra-index will not work, for example
+# the latest cython will be picked up from PyPI, rather than the one from the
+# python-nogil index


When I run the docker run -v $PWD:/scikit-learn ... command this header gets replaced. Maybe we move this comment into python_nogil_requirements.txt?

Maybe good enough for now? Longer term, I was thinking about using Docker to generate all the pip-based lock files.

I ended up doing what you suggested (I added the --upgrade option as well which is needed to upgrade dependencies as I discovered recently)

…nto ci-lock-file

thomasjpfan

Codewise LGTM

Assuming we have an auto updating bot that opens a PR once a week to bump versions, what do you see the workflow look like when the PR fails?

lesteve · 2022-05-12T15:32:09Z

Assuming we have an auto updating bot that opens a PR once a week to bump versions, what do you see the workflow look like when the PR fails?

I guess a human has to investigate to understand the root cause of the issue. It might be simpler to fix it "by hand" (e.g. pin a package to a previous version) and regenerate the lock files with the script. Of course I am open to better suggestions. I am hoping that in most cases the PR updating the dependencies will pass.

I had in mind to use the peter-evans/create-pull-request. The defaults behaviour is what we want I think:

This only opens one PR which gets updated continuously until it is merged or
closed

thomasjpfan · 2022-05-13T15:10:56Z

Here is a concrete example: Imagine we have lock files in place, and pytest 6.2.5 was the latest locked version. The bot opens a PR trying to update to 7.0, which fails. Here are the options I see for a human to do:

(Quickest) Pin pytest to 6.2.5 in update_environments_and_lock_files.py and regenerate the lock files.
Fix the pytest error in code so it works with 7.0 and run update_environments_and_lock_files.py.

In both cases, we end up closing the PR created by the bot.

lesteve · 2022-05-13T15:24:02Z

I am fine with your proposal.

In both cases, we end up closing the PR created by the bot.

It seems OK to me. Maybe you were hoping for a better workflow?

I think the main advantage of the CI lock files is that we don't have to rush to fix it since other PRs and main will be unaffected by the issue.

thomasjpfan · 2022-05-13T15:51:00Z

I think the main advantage of the CI lock files is that we don't have to rush to fix it since other PRs and main will be unaffected by the issue.

I'm on board with lock files. I'm trying to see what situations we end up with when we have lock files. Let's say a new version of NumPy and pytest are both causing errors. The most straight forward thing to do is to pin both of them in update_environments_and_lock_files and a contributor can work on one at a time.

There are a few follow up items to this PR:

Configure bot to create PR to auto update.
Document steps for a contributor to take if the PR fails.

lesteve · 2022-05-13T17:29:03Z

There are a few follow up items to this PR:

Yep. On top of the things you mention, I also had in mind to switch to lock files:

in Azure Windows builds
in CircleCI builds

…rn#22448) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

First attempt at using lock files in the CI

36279ff

github-actions bot added the Build / CI label Feb 11, 2022

lesteve added 7 commits February 11, 2022 11:13

fix

dafedd6

debug

cc3d965

it

1a08f1a

fix

6a74cb1

fix

c2ae825

fix

a23dc48

Need openmp in OSX build

6a47b49

Merge branch 'main' into ci-lock-file

e53f6fa

lesteve added 2 commits February 16, 2022 18:09

Use lock file in py37_conda_defaults_openblas build.

d2a6dea

Also regenerate lock files

add missing lock and env file

86638b8

fix lock filename

e890a4b

lesteve added 7 commits February 18, 2022 19:21

Switch to conda-lock dev version + regenerate lock files

854c735

Fix lock filenames

08fc05a

Using conda-lock.yml at the end of the file seems to make conda-lock …

4a41fdf

…happier

debug with tmate

9b793d5

Try conda update

c987848

Fix macOS

d0e9115

debug

ab2337e

lesteve mentioned this pull request Feb 21, 2022

CI: refactor Azure CI install script #22567

Merged

[scipy-dev] [pypy] [icc-build] Trigger CI

6fc77e5

thomasjpfan reviewed May 4, 2022

View reviewed changes

lesteve added 4 commits May 9, 2022 10:59

Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…

e56334a

…nto ci-lock-file

[scipy-dev] [pypy] [icc-build] [nogil] Trigger CI

8cf6d64

Update lock files

c5e8be9

wip

4edb574

lesteve mentioned this pull request May 9, 2022

Use docker image for nogil CI #23312

Closed

lesteve added 2 commits May 9, 2022 18:12

[scipy-dev] [pypy] [icc-build] [nogil] Add lock files for python-nogil

054a0d3

[scipy-dev] [pypy] [icc-build] [nogil] Fix

7c4f4a1

thomasjpfan reviewed May 10, 2022

View reviewed changes

lesteve added 3 commits May 11, 2022 12:05

Remove nogil build from script

fb1d21f

Use --upgrade to for pip-compile to update dependencies

d8c11e4

Rerun script

b81fea9

thomasjpfan reviewed May 11, 2022

View reviewed changes

lesteve added 2 commits May 12, 2022 11:22

Move comment to requirement file + add --upgrade

2f4ebdc

Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…

66f0f07

…nto ci-lock-file

thomasjpfan approved these changes May 12, 2022

View reviewed changes

thomasjpfan merged commit f862129 into scikit-learn:main May 13, 2022

lesteve deleted the ci-lock-file branch May 16, 2022 09:58

glemaitre pushed a commit that referenced this pull request Aug 5, 2022

CI: move Linux and MacOS Azure builds to conda lock files (#22448)

964435f

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

lesteve mentioned this pull request May 17, 2023

CI Update nogil lock file #26388

Merged

lesteve mentioned this pull request Jan 3, 2025

DEV add missing dep to lock-file script docstring #30574

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: move Linux and MacOS Azure builds to conda lock files #22448

CI: move Linux and MacOS Azure builds to conda lock files #22448

lesteve commented Feb 11, 2022

thomasjpfan commented Feb 11, 2022

lesteve commented Feb 11, 2022

lesteve commented Feb 11, 2022 •

edited

Loading

thomasjpfan commented Feb 13, 2022

ogrisel commented Feb 15, 2022

lesteve commented Feb 16, 2022

lesteve commented Feb 16, 2022

lesteve commented Feb 16, 2022 •

edited

Loading

ogrisel commented Feb 18, 2022

thomasjpfan left a comment

lesteve commented May 10, 2022

ogrisel commented May 10, 2022

thomasjpfan left a comment

thomasjpfan May 10, 2022

lesteve May 11, 2022

thomasjpfan May 11, 2022

lesteve May 12, 2022

lesteve May 12, 2022

thomasjpfan left a comment

lesteve commented May 12, 2022

thomasjpfan commented May 13, 2022 •

edited

Loading

lesteve commented May 13, 2022

thomasjpfan commented May 13, 2022

lesteve commented May 13, 2022

	if [[ "$PYTEST_XDIST_VERSION" != "none" ]]; then
	TEST_CMD="$TEST_CMD -n$CPU_COUNT"

CI: move Linux and MacOS Azure builds to conda lock files #22448

CI: move Linux and MacOS Azure builds to conda lock files #22448

Conversation

lesteve commented Feb 11, 2022

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments

thomasjpfan commented Feb 11, 2022

lesteve commented Feb 11, 2022

lesteve commented Feb 11, 2022 • edited Loading

thomasjpfan commented Feb 13, 2022

ogrisel commented Feb 15, 2022

lesteve commented Feb 16, 2022

lesteve commented Feb 16, 2022

lesteve commented Feb 16, 2022 • edited Loading

ogrisel commented Feb 18, 2022

thomasjpfan left a comment

Choose a reason for hiding this comment

lesteve commented May 10, 2022

ogrisel commented May 10, 2022

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan May 10, 2022

Choose a reason for hiding this comment

lesteve May 11, 2022

Choose a reason for hiding this comment

thomasjpfan May 11, 2022

Choose a reason for hiding this comment

lesteve May 12, 2022

Choose a reason for hiding this comment

lesteve May 12, 2022

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

lesteve commented May 12, 2022

thomasjpfan commented May 13, 2022 • edited Loading

lesteve commented May 13, 2022

thomasjpfan commented May 13, 2022

lesteve commented May 13, 2022

lesteve commented Feb 11, 2022 •

edited

Loading

lesteve commented Feb 16, 2022 •

edited

Loading

thomasjpfan commented May 13, 2022 •

edited

Loading