Skip to content

CI Create environment with conda rather than conda-lock when possible #29176

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

Merged
merged 18 commits into from
Jun 6, 2024

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Jun 4, 2024

Because we are using explicit lock-files, this is a format that conda knows about and there is no need to install conda-lock to create the environment if you only have conda package in the lock-file. If you have pip package in the lock-file you need to use conda-lock.

For more details, see https://conda.github.io/conda-lock/output/#explicit-lockfile.

This PR also cleans up a few things:

Not that crucial, but this may speed up things a tiny bit (~20s on my machine) for a few builds:

  • no need to download the conda-forge metadata
  • no need to install conda-lock

Copy link

github-actions bot commented Jun 4, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 6e5b2d7. Link to the linter CI: here

@lesteve lesteve marked this pull request as ready for review June 4, 2024 13:19
Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

LGTM

MINIFORGE_PATH=$HOME/miniforge3
bash ./miniconda.sh -b -p $MINIFORGE_PATH
source $MINIFORGE_PATH/etc/profile.d/conda.sh
conda activate
Copy link
Member

Choose a reason for hiding this comment

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

for CI, micromamba is a lot smaller and quite fast. Would it be okay to use it or you rather not bother?

Copy link
Member Author

Choose a reason for hiding this comment

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

I seem to remember that you had issues with updating the lock-files though with micromamba, right, and that the fix for you was micromamba install mamba right? My wild guess is that there were issues with virtual packages that were fixed in conda (I did one of the PR by the way). Because micromamba reimplements these parts, they were issues, there is a small chance that someone reported it and that it was fixed.

I am not convinced the "lot smaller" this matters that much in practice for the, CI maybe you save a few seconds for the downloads and 2-3 lines for the installation code.

All in all, I would be in favour of being conservative here.

Copy link
Member

Choose a reason for hiding this comment

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

The solution for me was to install both mamba and conda to be able to update lock files.

The smaller thing is significant since micromamba doesn't come with a base environment. So there's one whole environment saved by not using conda here.

Copy link
Member

Choose a reason for hiding this comment

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

I think this PR is already a net improvement. Can we discuss micromamba in a follow up PR?

MINIFORGE_PATH=$HOME/miniforge3
bash ./miniconda.sh -b -p $MINIFORGE_PATH
source $MINIFORGE_PATH/etc/profile.d/conda.sh
conda activate
Copy link
Member

Choose a reason for hiding this comment

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

I think this PR is already a net improvement. Can we discuss micromamba in a follow up PR?

@betatim
Copy link
Member

betatim commented Jun 5, 2024

If you want to Loïc we could update

# XXX switch once https://github.com/scikit-learn/scikit-learn/pull/29176 is merged
in this PR. Otherwise I can make a new PR as well to do it

@lesteve
Copy link
Member Author

lesteve commented Jun 6, 2024

I updated scikit-learn/build_tools/github/create_gpu_environment.sh, you probably are aware of this, but this is not tested by the PR CI 😉.

Having said that the changes look simple enough, so it should be fine.

@adrinjalali adrinjalali merged commit a605b00 into scikit-learn:main Jun 6, 2024
30 checks passed
@jeremiedbb jeremiedbb mentioned this pull request Jul 2, 2024
11 tasks
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.

4 participants