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

Extract mpi tests from integration CI as independent CI #3606

Merged
merged 2 commits into from
Jun 1, 2022

Conversation

nzw0301
Copy link
Member

@nzw0301 nzw0301 commented May 31, 2022

Motivation

Resolve #3570

Description of the changes

Extract MPI's pytest from .github/workflows/tests-integration.yml and create a new yaml file for MPI test.

@nzw0301 nzw0301 added the CI Continuous integration. label May 31, 2022

pip install --progress-bar off .[test]
pip install --progress-bar off .[optional]
pip install --progress-bar off .[integration] --extra-index-url https://download.pytorch.org/whl/cpu
Copy link
Member Author

Choose a reason for hiding this comment

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

My concern is that this line installs unnecessary other integration packages.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is acceptable for this PR. I guess @himkt is interested in re-organization of setup.py and we can discuss this topic there.

@codecov-commenter
Copy link

Codecov Report

Merging #3606 (968fb47) into master (f56cea8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3606   +/-   ##
=======================================
  Coverage   90.83%   90.83%           
=======================================
  Files         161      161           
  Lines       12392    12392           
=======================================
  Hits        11256    11256           
  Misses       1136     1136           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

Thank you for your PR. I basically looks great to me, but I have a small question.

.github/workflows/tests-mpi.yml Show resolved Hide resolved

pip install --progress-bar off .[test]
pip install --progress-bar off .[optional]
pip install --progress-bar off .[integration] --extra-index-url https://download.pytorch.org/whl/cpu
Copy link
Member

Choose a reason for hiding this comment

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

I think it is acceptable for this PR. I guess @himkt is interested in re-organization of setup.py and we can discuss this topic there.

@toshihikoyanase
Copy link
Member

@HideakiImamura Could you review this PR as an author of #3570, please?

Copy link
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks great.

@toshihikoyanase toshihikoyanase added this to the v3.0.0-b1 milestone Jun 1, 2022
Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you.

@toshihikoyanase toshihikoyanase merged commit 96b7fe7 into optuna:master Jun 1, 2022
@toshihikoyanase
Copy link
Member

By the way, do we update the branch protection rule to make tests-mpi required? @HideakiImamura

@HideakiImamura
Copy link
Member

By the way, do we update the branch protection rule to make tests-mpi required?

Agree. May I update it required?

@nzw0301 nzw0301 deleted the separate-mpi branch June 1, 2022 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MPI tests within the integration module tests should be run separately from the rest
4 participants