Skip to content
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

CI Adds quicker CI failure to reduce resource usage #21497

Merged
merged 47 commits into from
Dec 13, 2022

Conversation

thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Oct 29, 2021

Reference Issues/PRs

Closes #21403

What does this implement/fix? Explain your changes.

This PR allows Ubuntu_Bionic to run first and have most of the other instances depend on it.

  1. I added a second Ubuntu_Bionic_Parallel that runs only when [azure all-parallel] is in the commit message. If [azure all-parallel] is not in the commit message, then Ubuntu_Bionic_Parallel is skipped.

  2. Ubuntu_Bionic only runs when [azure all-parallel] is not in the commit message.

  3. The other instances are updated to run when Ubuntu_Bionic succeeds or skipped, which covers all the non-failure cases.

@ogrisel
Copy link
Member

ogrisel commented Oct 29, 2021

Could you please add a top level comment in this file to explain the logic behind this dependency graph?

- template: build_tools/azure/posix.yml
parameters:
name: Linux
vmImage: ubuntu-20.04
name: Ubuntu_Bionic_Parallel
Copy link
Member

Choose a reason for hiding this comment

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

Instead of introducing this redundant build entry, we could just change the condition on the other builds to use a dis-junction such as:

or (
    in(dependencies['Ubuntu_Bionic']['result'], 'Succeeded', 'Skipped'),
    contains(dependencies['git_commit']['outputs']['commit.message'], '[azure all-parallel]')
)

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 am unable to get this to work without the redundant build. The other builds are configured with dependsOn: [git_commit, linting, Ubuntu_Bionic], which waits for Ubuntu_Bionic to complete.

@ogrisel
Copy link
Member

ogrisel commented Oct 29, 2021

I think we should use Linux_Latest pylatest_conda_forge_mkl as the default run which is the fastest (runs in 15min total). We might even make it run even faster by using openblas instead of MKL which is slow to download (not sure about the impact of OpenBLAS vs MKL on the tests and use MKL in another linux run.

Edit: Scratch this last comment. On the latest run the bionic job was the fastest.

doc/developers/contributing.rst Outdated Show resolved Hide resolved
build_tools/azure/posix-all-parallel.yml Outdated Show resolved Hide resolved
build_tools/azure/posix-all-parallel.yml Outdated Show resolved Hide resolved
azure-pipelines.yml Show resolved Hide resolved
@thomasjpfan thomasjpfan changed the title MNT Adds quicker CI failure to reduce resource usage CI Adds quicker CI failure to reduce resource usage Dec 3, 2022
@thomasjpfan
Copy link
Member Author

After looking at this PR again, I reworked the comments in the yaml files to better explain what is going on. Here are the output of the CI:

  • Azure CI with the azure parallel flag: Ubuntu_Jammy_Jellyfish_Parallel job runs in parallel with the other jobs:

Screen Shot 2022-12-03 at 3 19 09 PM

  • Azure CI without the flag, the Ubuntu_Jammy_Jellyfish runs first and the other jobs are waiting for Ubuntu_Jammy_Jellyfish to succeed:

Screen Shot 2022-12-03 at 3 25 14 PM

Copy link
Member

@jjerphan jjerphan 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 comments are much simpler and clearer to me know. Thank you for making the CI usage less resources, @thomasjpfan.

@jjerphan jjerphan added the Quick Review For PRs that are quick to review label Dec 12, 2022
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, thanks for the improvement. Hopefully this will reduce our CI usage.

If this strategy proves to be annoying or too complex to understand, we can always revert.

@ogrisel ogrisel merged commit e1ec3f9 into scikit-learn:main Dec 13, 2022
Vincent-Maladiere pushed a commit to Vincent-Maladiere/scikit-learn that referenced this pull request Dec 14, 2022
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
jjerphan added a commit to jjerphan/scikit-learn that referenced this pull request Jan 3, 2023
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
jjerphan added a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
jjerphan added a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
jjerphan added a commit to jjerphan/scikit-learn that referenced this pull request Jan 23, 2023
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
adrinjalali pushed a commit that referenced this pull request Jan 24, 2023
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this pull request Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build / CI module:metrics Quick Review For PRs that are quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quicker Failing CI with reduced resource usage
3 participants