-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
CI: refactor Azure CI install script #22567
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for cleaning this up! Overall, I like the refactor.
build_tools/azure/install.sh
Show resolved
build_tools/azure/install.sh
Outdated
# The "build_clib" command is implicitly used to build "libsvm-skl". | ||
# To compile with a different compiler, we also need to specify the | ||
# compiler for this command | ||
python setup.py build_ext --compiler=intelem -i build_clib --compiler=intelem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still have to wait till scikit_learn_install
to run build_ext
so we use the correct python environment?
Currently on main
, the conda environment is created first and setup.py build_ext
is called in that enviornment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch I fixed this and use [icc-build]
in my last commit to make sure to test the BUILD_WITH_ICC logic and the CI passes (here is the log)
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
plus move post_pyton_environment_install to scikit_learn_install
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well!
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
The CI is green merging, thanks for the reviews! |
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
The main goal is to make it easier to move to lock files #22448, i.e. to isolate Python environment creation from the rest.
This makes it easier to grok at the cost of making it a bit harder to follow what is happening in a particular build.
I split the install in four steps:
pre_python_environment_install
: apt-get and other similar thingspython_environment_install
: installing the python environment as the name says-I ended up merging this step withpost_python_environment_install
:setup_ccache
and showing installed libraries. It could be removed and two lines added to the beginning ofscikit_learn_install
if deemed preferrablescikit_learn_install
scikit_learn_install
: environment variable setup for compilation, whether or not to use build isolation,ccache -s
afterwards