Skip to content

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

Merged
merged 27 commits into from
Oct 9, 2021
Merged

Conversation

jjerphan
Copy link
Member

@jjerphan jjerphan commented Sep 28, 2021

Reference Issues/PRs

Extracted from #20254.

What does this implement/fix? Explain your changes.

The script now:

  • uses Mamba-forge to create a Python 3.9 environment
  • installs the latest dependencies by default
  • disables isolation when building
  • redefines caches to reuse builds'

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)).

The script now uses:
 - uses Mamba-forge to create a Python 3.7 environment
 - installs the latest dependencies by default
 - disable isolation when building
@ogrisel
Copy link
Member

ogrisel commented Sep 28, 2021

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>
@ogrisel
Copy link
Member

ogrisel commented Sep 28, 2021

@jjerphan do you recall why you needed to use mamba/conda-forge instead of pip/pypi.org to make it work for #20254? Was it a problem with the openblas version installed by pip?

@jjerphan
Copy link
Member Author

jjerphan commented Sep 28, 2021

@jjerphan do you recall why you needed to use mamba/conda-forge instead of pip/pypi.org to make it work for #20254? Was it a problem with the openblas version installed by pip?

Yes, it was (#20254 (comment)).

jjerphan and others added 4 commits September 28, 2021 11:36
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Motives: It works with this.
Note that's what used in all other CI scripts.
@ogrisel
Copy link
Member

ogrisel commented Sep 28, 2021

If we build in editable mode, we should use --no-build-isolation to avoid re-downloading the build dependencies.

But I think @glemaitre's point was to build / install without editable, then cd /tmp && pytest --pyargs sklearn to launch the test on the result of the installation.

Copy link
Contributor

@cmarmo cmarmo left a 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... :)

@jjerphan
Copy link
Member Author

@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.

jjerphan and others added 2 commits September 29, 2021 10:04
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com>
jjerphan and others added 2 commits September 29, 2021 10:15
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
jjerphan and others added 2 commits September 29, 2021 10:40
@cmarmo
Copy link
Contributor

cmarmo commented Sep 29, 2021

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)).

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?

@jjerphan
Copy link
Member Author

The last Circle CI log show better but still low cache hit-rate:

+ ccache -s --verbose
Summary:
  Cache directory:    /home/circleci/.cache/ccache
  Primary config:     /home/circleci/.config/ccache/ccache.conf
  Secondary config:   /home/circleci/miniconda/envs/testenv/etc/ccache.conf
  Stats updated:      Thu Sep 30 14:09:40 2021
  Hits:                 15 /  126 (11.90 %)
    Direct:              6 /  126 (4.76 %)
    Preprocessed:        9 /  120 (7.50 %)
  Misses:              111
    Direct:            120
    Preprocessed:      111
  Uncacheable:         124
Primary storage:
  Hits:                 21 /  252 (8.33 %)
  Misses:              231
  Cache size (GB):    0.01 / 0.00
  Files:               231
Uncacheable:
  Called for linking:  118
  No input file:         6

I don't know what's wrong with the setup.

@ogrisel
Copy link
Member

ogrisel commented Oct 1, 2021

What ccache stats do you get if you rebuild scikit-learn from a new git clone in the same folder locally?

@jjerphan
Copy link
Member Author

jjerphan commented Oct 1, 2021

A bigger cache is populated and comes with a higher cache hit rate:

cache directory                     /home/jjerphan/.cache/ccache
primary config                      /home/jjerphan/.config/ccache/ccache.conf
secondary config (readonly)         /etc/ccache.conf
stats updated                       Fri Oct  1 12:05:25 2021
cache hit (direct)                   111
cache hit (preprocessed)               0
cache miss                           105
cache hit rate                     51.39 %
called for link                      120
no input file                          5
cleanups performed                     0
files in cache                       210
cache size                           6.5 MB

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
jjerphan and others added 4 commits October 7, 2021 12:05
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Also be more specific for the source build folder.
@jjerphan
Copy link
Member Author

jjerphan commented Oct 7, 2021

@thomasjpfan's proposal with some supplementary caching (b0572b7) solved the problem.

+ ccache -s --verbose
Summary:
  Cache directory:    /home/circleci/.cache/ccache
  Primary config:     /home/circleci/.config/ccache/ccache.conf
  Secondary config:   /home/circleci/miniconda/envs/testenv/etc/ccache.conf
  Stats updated:      Thu Oct  7 11:41:05 2021
  Hits:                 65 /  138 (47.10 %)
    Direct:             65 /  139 (46.76 %)
    Preprocessed:        0 /   74 (0.00 %)
  Misses:               73
    Direct:             74
    Preprocessed:       74
  Uncacheable:         123
Primary storage:
  Hits:                130 /  278 (46.76 %)
  Misses:              148
  Cache size (GB):    0.01 / 0.00
  Files:               146
Uncacheable:
  Called for linking:  118
  Compilation failed:    1
  No input file:         4

Build are faster by about 5 minutes (it took 18m 35s before but takes 13m 25s now).

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!

@ogrisel
Copy link
Member

ogrisel commented Oct 8, 2021

@thomasjpfan merge?

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +141 to +142
# The source build folder.
- ~/project/build
Copy link
Member

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.

Copy link
Member

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 :)

@thomasjpfan thomasjpfan merged commit b2ee0f4 into scikit-learn:main Oct 9, 2021
@glemaitre glemaitre mentioned this pull request Oct 23, 2021
10 tasks
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Oct 23, 2021
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>
glemaitre added a commit that referenced this pull request Oct 25, 2021
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>
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
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>
@jjerphan jjerphan deleted the circle-ci-arm branch October 21, 2022 14:03
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.

5 participants