Skip to content

MNT Centralize conda-lock version into min_dependencies.py #23432

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 4 commits into from
May 23, 2022

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Related to #22425

What does this implement/fix? Explain your changes.

This PR moves the pinned version of conda-lock into min_dependencies.py and grabs the version from there.

WDYT @lesteve ?

extra: []
for extra in ["build", "install", "docs", "examples", "tests", "benchmark"]
}
tag_to_packages: dict = defaultdict(list)
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 makes it easier to add new entries, such as "maintenance" .

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM!

@lesteve
Copy link
Member

lesteve commented May 23, 2022

I agree that it's nicer to have a single place where the conda-lock version is defined.

I have to say I would slightly prefer to have export CONDA_LOCK_VERSION=1.0.5 in build_tools/shared.sh, which I find a bit simpler.

Do you have a use case in mind where it is useful to have this in _min_dependencies (and thus in setup.py), e.g. to do pip install scikit-learn[maintenance] (I don't think the latter is implemented but it could)

@thomasjpfan
Copy link
Member Author

I prefer to have all the versions in a centralized place. Placing it all in _min_dependencies allows access to this information through Python or bash. With CONDA_LOCK_VERSION, we would need to access it through bash and an env variable.

Do you have a use case in mind where it is useful to have this in _min_dependencies

I think a simple use case is for update_environments_and_lock_files.py to check the conda-lock version is correct before running conda-lock. I think this is slightly easier to do if the conda-lock is in _min_dependencies.

@lesteve lesteve merged commit ef4c26b into scikit-learn:main May 23, 2022
@lesteve
Copy link
Member

lesteve commented May 23, 2022

OK, this makes sense, merging this one!

I do find it a bit weird in the sense that the conda-lock version is not really a min dependency like the others, but this is definitely an improvement compared to having to bump the conda-lock version in multiple files.

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

Successfully merging this pull request may close these issues.

3 participants