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

Fix checks integration about pytorch lightning #4322

Conversation

Alnusjaponica
Copy link
Collaborator

@Alnusjaponica Alnusjaponica commented Jan 12, 2023

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

codecov-commenter commented Jan 12, 2023

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 marked this pull request as ready for review January 13, 2023 09:23
@Alnusjaponica
Copy link
Collaborator Author

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
Collaborator Author

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
Contributor

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
@@ -96,7 +94,7 @@ def on_validation_end(self, trainer: Trainer, pl_module: LightningModule) -> Non
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
Member

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
Collaborator Author

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
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.

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
Collaborator Author

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

@Alnusjaponica
Copy link
Collaborator Author

I added follow-up PR #4384

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.

LGTM.

@HideakiImamura HideakiImamura removed their assignment Feb 6, 2023
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.

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 deleted the fix-checks-integration-pytorch-lightning-alternative branch February 10, 2023 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Support latest PyTorch lightning
5 participants