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

Do not run the GC after every trial by default. #1380

Merged
merged 2 commits into from Jun 17, 2020

Conversation

hvy
Copy link
Member

@hvy hvy commented Jun 16, 2020

Motivation

Fixes #1307. See this issue for discussions.

Description of the changes

Changes the default GC collection behavior of Study.optimize to not run a collection.

@codecov-commenter
Copy link

Codecov Report

Merging #1380 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1380      +/-   ##
==========================================
- Coverage   88.08%   88.06%   -0.03%     
==========================================
  Files          97       97              
  Lines        7313     7297      -16     
==========================================
- Hits         6442     6426      -16     
  Misses        871      871              
Impacted Files Coverage Δ
optuna/study.py 94.71% <100.00%> (-0.17%) ⬇️
optuna/_study_summary.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a652d04...43a4821. Read the comment docs.

@HideakiImamura HideakiImamura self-assigned this Jun 17, 2020
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 think we need to set gc_after_trial=True when we use ChainerMNStudy (c.f., #325). I'm not sure if this issue affects the GitHub Actions, though.

@toshihikoyanase toshihikoyanase self-assigned this Jun 17, 2020
@hvy
Copy link
Member Author

hvy commented Jun 17, 2020

Is it not addressed by https://github.com/optuna/optuna/blob/master/optuna/integration/chainermn.py#L146? I was also wondering whether this line should be kept or controllable similar to the standard Study.

@toshihikoyanase
Copy link
Member

The line is executed by non-rank-0 nodes, but I think we need to call GC for rank-0 node. The rank-0 node calls Study.optimize without gc_after_trial option at https://github.com/optuna/optuna/blob/master/optuna/integration/chainermn.py#L122. And I think it may cause similar issues to #325.

this line should be kept or controllable similar to the standard Study.

Depending on the user code and environments, GC calls can be omitted. I support the latter approach, but I think we can work on it in a new PR.

@hvy
Copy link
Member Author

hvy commented Jun 17, 2020

Thanks for your explanation. I now see what you meant, that the behavior of ChainerMNStudy will be different for different workers, that only the first worker will not run the GC collection. However, as the underlying motivation behind this change is to leave the memory handling to the users, I suggest we stop running the GC in the ChainerMNStudy as well. Let me change it that way (discussed with @toshihikoyanase).

@hvy
Copy link
Member Author

hvy commented Jun 17, 2020

PTAL.

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 the fruitful discussion about ChainerMN. I understood the motivation of this PR better.

LGTM.

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 and the reasonable discussion. LGTM!

@HideakiImamura HideakiImamura merged commit 777735d into optuna:master Jun 17, 2020
@hvy hvy deleted the fix-gc-after-trial-default-false branch June 17, 2020 08:06
@hvy hvy added this to the v2.0.0 milestone Jun 17, 2020
@hvy hvy added the enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. label Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Change that does not break compatibility and not affect public interfaces, but improves performance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Change the default argument of gc_after_trial in Study.optimize to False.
4 participants