Skip to content

Fix checks integration about pytorch lightning#4322

Merged
toshihikoyanase merged 10 commits intooptuna:masterfrom
Alnusjaponica:fix-checks-integration-pytorch-lightning-alternative
Feb 9, 2023
Merged

Fix checks integration about pytorch lightning#4322
toshihikoyanase merged 10 commits intooptuna:masterfrom
Alnusjaponica:fix-checks-integration-pytorch-lightning-alternative

Conversation

@Alnusjaponica
Copy link
Copy Markdown
Contributor

@Alnusjaponica Alnusjaponica commented Jan 12, 2023 •
edited
Loading

Motivation

Resolve #3418 and #4116

Description of the changes

Refactor deprecated features

  • trainer.training_type_plugin is deleted since v1.8 (PR#11239). The attribute training_type_plugin is just renamed to strategy, so refactored as suggested.

  • An optional argument of trainer, accelerator stopped to accept ddp_cpu. Instead, we can pass ddp to strategy and cpu to accelerator. Also, num_processes will be removed, so we give the number of processes to devices instead.

  • AcceleratorConnector.distributed_backend is deleted, but now they have AcceleratorConnector.is_distributed instead, so refactored as suggested.

  • callback.on_init_start() is deleted since v1.8 (Issue#10894, PR#10940, PR#14867).
    Although they don't provide exactly equivalent alternative, it would be possible to move this confirmation to somewhere else. Although it seems strategy.setup_environment is the right place to implement this check, implementing as a method of Strategy will affect user's code. Therefore it will be reasonable to implement as callback.setup or callback.on_fit_start().

Stop supporting DDP temporary

When you use DDP and optuna.TrialPruned() is raised from the child process, Pytorch Lightning tries to resynchronize to fix the "error" and finally deal it as a DeadlockDetectedException, which terminates the whole process. For more details, see reconciliate_processes. To fix this problem, we need to change environment variable and private variable.

It might be possible to solve this problem by manually raising optuna.TrialPruned() from objective function as in CatBoostPruningCallback (see this comment). If it is possible, I am going to apply the change as another PR.

  • This PR skips test_pytorch_lightning_pruning_callback_ddp_monitor and test_pytorch_lightning_pruning_callback_ddp_unsupported_storage.

@github-actions github-actions bot added the optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions. label Jan 12, 2023
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 12, 2023 •
edited
Loading

Codecov Report

Merging #4322 (eab1a89) into master (765143e) will increase coverage by 0.00%.
The diff coverage is 0.00%.

📣 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    #4322   +/-   ##
=======================================
  Coverage   90.43%   90.43%           
=======================================
  Files         172      172           
  Lines       13660    13660           
=======================================
+ Hits        12353    12354    +1     
+ Misses       1307     1306    -1
Impacted Files Coverage Δ
optuna/integration/pytorch_lightning.py 0.00% <0.00%> (ø)
optuna/progress_bar.py 87.69% <0.00%> (+1.53%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Alnusjaponica Alnusjaponica mentioned this pull request Jan 13, 2023
Closed
@Alnusjaponica Alnusjaponica marked this pull request as ready for review January 13, 2023 09:23
@Alnusjaponica Alnusjaponica mentioned this pull request Jan 15, 2023
Merged
@Alnusjaponica
Copy link
Copy Markdown
Contributor Author

Alnusjaponica commented Jan 15, 2023

Adding changes in this PR, I confirmed that all mypy test in Checks (integration) passes. See following CI on my fork.
Although Tests (MPI) failed in the fork, this test has already failed on the master branch. Therefore, the cause should lie somewhere independent of this change.

@Alnusjaponica
Copy link
Copy Markdown
Contributor Author

Alnusjaponica commented Jan 19, 2023

I made this PR review ready. I appreciate it if you could assign reviews.
@toshihikoyanase

@toshihikoyanase toshihikoyanase self-assigned this Jan 19, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 26, 2023

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Jan 26, 2023
@contramundum53 contramundum53 added the CI Continuous integration. label Jan 27, 2023
toshihikoyanase
toshihikoyanase reviewed Jan 27, 2023
View reviewed changes
optuna/integration/pytorch_lightning.py
if trainer.is_global_zero:
self._trial.report(current_score.item(), step=epoch)
should_stop = self._trial.should_prune()
should_stop = trainer.training_type_plugin.broadcast(should_stop)
Copy link
Copy Markdown
Member

@toshihikoyanase toshihikoyanase Jan 27, 2023

Choose a reason for hiding this comment

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

I think we should update the version check at L64 from 1.5.0 to 1.6.0 since pytorch-lightning==1.5.0 does not have Trainer.strategy.

pytorch-lightning version optuna-examples/pytorch/pytorch_lightning_simple.py
1.5.0 NG
1.6.0 OK
1.7.0 OK
1.8.0 OK

The error message during the pytorch-lightning==1.5.0 was as follows:

AttributeError: 'Trainer' object has no attribute 'strategy'

Copy link
Copy Markdown
Contributor Author

@Alnusjaponica Alnusjaponica Jan 29, 2023

Choose a reason for hiding this comment

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

Thank you for your comment. I updated the version constraint.

@github-actions github-actions bot removed the stale Exempt from stale bot labeling. label Jan 29, 2023
@HideakiImamura HideakiImamura self-assigned this Jan 30, 2023
HideakiImamura
HideakiImamura reviewed Jan 30, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Basically, LGTM. Let me ask one question. on_fit_start includes the logic when the backend is DDP, but we do not test it. Do we have a plan to support the DDP (maybe, as a follow-up of this PR)?

@Alnusjaponica
Copy link
Copy Markdown
Contributor Author

Alnusjaponica commented Jan 30, 2023

Thanks for your comment. I am working on supporting DDP here and will create follow-up PR based on this.

@Alnusjaponica Alnusjaponica mentioned this pull request Feb 2, 2023
Merged
@Alnusjaponica
Copy link
Copy Markdown
Contributor Author

Alnusjaponica commented Feb 2, 2023

I added follow-up PR #4384

HideakiImamura
HideakiImamura approved these changes Feb 6, 2023
View reviewed changes
Copy link
Copy Markdown
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.

LGTM.

@HideakiImamura HideakiImamura removed their assignment Feb 6, 2023
toshihikoyanase
toshihikoyanase approved these changes Feb 9, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@toshihikoyanase toshihikoyanase left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry for the delayed response. pytorch_lightning_simple.py in the optuna example worked with the latest pytorch lightning as expected. We my update the lower bound of pythorch lightning in the example from 1.5.0 to 1.6.0, it is a follow-up task.

LGTM!

@toshihikoyanase toshihikoyanase added code-fix Change that does not change the behavior, such as code refactoring. and removed CI Continuous integration. labels Feb 9, 2023
@toshihikoyanase toshihikoyanase added this to the v3.2.0 milestone Feb 9, 2023
@toshihikoyanase toshihikoyanase merged commit 17ff642 into optuna:master Feb 9, 2023
@Alnusjaponica Alnusjaponica mentioned this pull request Feb 10, 2023
Merged
@Alnusjaponica Alnusjaponica deleted the fix-checks-integration-pytorch-lightning-alternative branch February 10, 2023 07:14
This was referenced Feb 16, 2023
Closed
Merged
@Alnusjaponica Alnusjaponica mentioned this pull request Mar 9, 2023
Closed
@nzw0301 nzw0301 mentioned this pull request Apr 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toshihikoyanase toshihikoyanase toshihikoyanase approved these changes

@HideakiImamura HideakiImamura HideakiImamura approved these changes

Assignees

@toshihikoyanase toshihikoyanase

Labels

code-fix Change that does not change the behavior, such as code refactoring. optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions.

Projects

None yet

Milestone

v3.2.0

Development

Successfully merging this pull request may close these issues.

Support latest PyTorch lightning

5 participants

@Alnusjaponica @codecov-commenter @toshihikoyanase @HideakiImamura @contramundum53