Delete optuna.integration.ChainerMNStudy for migrating to optuna-integration package#4497
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #4497 +/- ##
==========================================
+ Coverage 90.04% 90.06% +0.01%
==========================================
Files 181 181
Lines 14026 13842 -184
==========================================
- Hits 12630 12467 -163
+ Misses 1396 1375 -21
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
c-bata
left a comment
There was a problem hiding this comment.
Changes overall look good to me. I left one question though.
| def test_module_attributes() -> None: | ||
| import optuna | ||
|
|
||
| assert hasattr(optuna.integration, "chainermn") |
There was a problem hiding this comment.
[question] Do you think we should remove these lines even though we leave the optuna.integration.chainermn module for backward compatibility. Or is it possible to install optuna-integration in tests-integration.yml and pass these tests?
There was a problem hiding this comment.
If we continue to conduct this test on the Optuna side, there will continue to be a dependency on Chainer, which is not desirable from the initial motivation of migrating the integration module. Therefore, I think the current approach of completely removing the dependency on Chainer from the test is appropriate. What do you think? @contramundum53 @gen740
There was a problem hiding this comment.
Does it?
I think we could check whether "chainermn" exists in optuna.integration without actually importing it.
There was a problem hiding this comment.
@gen740 pointed out that simply calling hasattr on "chainermn" would cause it to be imported. I think removing the test is Okay.
|
@contramundum53 Could you review this PR? |
contramundum53
left a comment
There was a problem hiding this comment.
LGTM except for @c-bata 's comment.
Motivation
Same motivation as #4370.
In this PR, I delete the
optuna.integration.ChainerMNStudyfor optuna-integration.Description of the changes
optuna.integration.ChainerMNStudyand its test.