-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CI Adapt Circle CI script for ARM #21174
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
The script now uses: - uses Mamba-forge to create a Python 3.7 environment - installs the latest dependencies by default - disable isolation when building
Could you please trigger a second build in this PR. I would like to check if the ccache statistics displayed at the end of the build show that ccache is effective with a high hit rate. If it's not the case it would be worth investigating. |
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Yes, it was (#20254 (comment)). |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Motives: It works with this. Note that's what used in all other CI scripts.
If we build in editable mode, we should use But I think @glemaitre's point was to build / install without editable, then |
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.
@jjerphan , thanks for your work.
I'd rather keep the pip check and trace the possible issues back, as pip is still the python package installer and it can be useful to improve its repository compatibility with arm64.
I guess the big question here is what is the goal of running checks on the code.... but I understand it's not my role to answer... :)
@cmarmo: Thanks for your comment. To give the context on this PR: patches (which have been extracted from #20254) were meant to use conda-forge via mamba instead of PyPi via pip to be able access the last OpenBLAS version for ARM64 because previous ones had problems for this architecture (#20254 (comment)). But do we always want to use conda-forge instead of PyPi? I also updated the PR description accordingly. |
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com>
cf91518
to
0a1c51d
Compare
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
0a1c51d
to
fd5eaff
Compare
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Thanks @jjerphan. I've seen the references. Python packages are also all installed via mamba (conda) in this PR, not only OpenBLAS. I guess this way, installing from PyPi is never tested for arm64? Well, probably on travis, but travis will be discarded at some point? |
c6c9527
to
96821dd
Compare
The last Circle CI log show better but still low cache hit-rate:
I don't know what's wrong with the setup. |
What ccache stats do you get if you rebuild scikit-learn from a new git clone in the same folder locally? |
A bigger cache is populated and comes with a higher cache hit rate:
|
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Also be more specific for the source build folder.
To trigger the build.
@thomasjpfan's proposal with some supplementary caching (b0572b7) solved the problem.
Build are faster by about 5 minutes (it took 18m 35s before but takes 13m 25s now). |
Also revert a patch.
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!
@thomasjpfan merge? |
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
# The source build folder. | ||
- ~/project/build |
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.
If we cache the whole build directory, I do not think we need ccache anymore. But we can leave it as is since the PR is already a net improvement.
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.
That's an interesting point :)
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Reference Issues/PRs
Extracted from #20254.
What does this implement/fix? Explain your changes.
The script now:
Any other comments?
Patches (which have been extracted from #20254) were meant to use conda-forge via mamba instead of PyPi via pip to be able access the last OpenBLAS version for ARM64 because previous ones had problems for this architecture (#20254 (comment)).