8000 CI Use lock files for Windows builds by lesteve · Pull Request #23379 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

CI Use lock files for Windows builds #23379

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 20, 2022

Conversation

lesteve
Copy link
Member
@lesteve lesteve commented May 16, 2022

Reference Issues/PRs

part of #22425

What does this implement/fix? Explain your changes.

This uses lock files for Windows builds in the CI

Any other comments?

For the Windows 32bit build, the assumption is that you can pin versions on a Linux 64bit machine. There is no cross-compile support for pip-compile (see https://github.com/jazzband/pip-tools#cross-environment-usage-of-requirementsinrequirementstxt-and-pip-compile for more details)

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, the script works well, although it seems that some dependencies have been released this since PR was opened:

diff --git a/build_tools/azure/py38_pip_openblas_32bit_lock.txt b/build_tools/azure/py38_pip_openblas_32bit_lock.txt
index 5dd3a5c04..b30f7c2c4 100644
--- a/build_tools/azure/py38_pip_openblas_32bit_lock.txt
+++ b/build_tools/azure/py38_pip_openblas_32bit_lock.txt
@@ -6,7 +6,7 @@
 #
 attrs==21.4.0
     # via pytest
-cython==0.29.28
+cython==0.29.30
     # via -r build_tools/azure/py38_pip_openblas_32bit_requirements.txt
 execnet==1.9.0
     # via pytest-xdist
@@ -20,7 +20,7 @@ numpy==1.22.3
     #   scipy
 packaging==21.3
     # via pytest
-pillow==9.1.0
+pillow==9.1.1
     # via -r build_tools/azure/py38_pip_openblas_32bit_requirements.txt
 pluggy==1.0.0
     # via pytest
@@ -39,7 +39,7 @@ pytest-forked==1.4.0
     # via pytest-xdist
 pytest-xdist==2.5.0
     # via -r build_tools/azure/py38_pip_openblas_32bit_requirements.txt
-scipy==1.8.0
+scipy==1.8.1
     # via -r build_tools/azure/py38_pip_openblas_32bit_requirements.txt
 threadpoolctl==3.1.0
     # via -r build_tools/azure/py38_pip_openblas_32bit_requirements.txt

@lesteve
Copy link
Member Author
lesteve commented May 18, 2022

I reran the script and pushed a commit to update the Windows build lock file.

@ogrisel
Copy link
Member
ogrisel commented May 18, 2022

I reran the script and pushed a commit to update the Windows build lock file.

And everything went fine :)

@glemaitre
Copy link
Member

@lesteve Could you solve the conflict since I merged the circle ci PR.

Copy link
Member
@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

@lesteve lesteve force-pushed the ci-lock-file-windows branch from 8b34fcb to b45b6d1 Compare May 19, 2022 16:31
if __name__ == "__main__":
@click.command()
@click.option(
"--select-build",
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 found this quite convenient when only generating a subset of all the possible lock files, regenerating all the lock files can take a while (~4 minutes on my machine)

Copy link
Member

Choose a reason for hiding this comment

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

This does add click as direct dependency. click is already installed because is a dependency for pip-tools. Let's add it into the list of dependencies?

To run this script you need:
- conda-lock. The version should match the one used in the CI in
build_tools/azure/install.sh
- pip-tools
- jinja2

Copy link
Member Author

Choose a reason for hiding this comment

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

click is a indirect dependency right? conda-lock (and pip-tools actually as you said) depends on click, so I thought it was not necessary to mention it.

Copy link
Member

Choose a reason for hiding this comment

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

jinja2 was included in the list as a indirect dependency from conda-lock. (I'm trying to be consistent with what goes in the 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.

I removed jinja2 from the list since it is a dependency of conda-lock

@glemaitre
Copy link
Member

activate is not recognize it seems

@lesteve
Copy link
Member Author
lesteve commented May 20, 2022

Merging this one!

@lesteve lesteve merged commit 84ad363 into scikit-learn:main May 20, 2022
@lesteve lesteve deleted the ci-lock-file-windows branch May 20, 2022 07:41
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Aug 4, 2022
glemaitre pushed a commit that referenced this pull request Aug 5, 2022
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
0