-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Upgrade to PTL 1.6.0 #3890
Upgrade to PTL 1.6.0 #3890
Conversation
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
This pull request introduces 1 alert when merging 55f7ede into ca8a7e0 - view on LGTM.com new alerts:
|
Signed-off-by: ericharper <complex451@gmail.com>
This pull request introduces 1 alert when merging 672c3ee into ca8a7e0 - view on LGTM.com new alerts:
|
Signed-off-by: ericharper <complex451@gmail.com>
This pull request introduces 1 alert when merging 58dadb6 into ca8a7e0 - view on LGTM.com new alerts:
|
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
Signed-off-by: ericharper <complex451@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, thanks ! Ive added a few todos we need to go back to - we need to revert setuptools patch next month (maybe) and definitely need to ask PTL for a stable way of accessing and overriding the connectors.
Accessing private variables inside ModelPT and expmanager is simply asking for bugs in the future
@@ -64,10 +64,10 @@ def main(cfg) -> None: | |||
exp_manager(trainer, cfg.exp_manager) | |||
|
|||
# update resume from checkpoint found by exp_manager | |||
resume_from_checkpoint = trainer.checkpoint_connector.resume_from_checkpoint_fit_path | |||
resume_from_checkpoint = trainer._checkpoint_connector.resume_from_checkpoint_fit_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo: Ask PTL to provide a stable public API for this
@@ -1030,7 +1030,7 @@ def maybe_init_from_pretrained_checkpoint(self, cfg: OmegaConf, map_location: st | |||
trainer = self.trainer | |||
if ( | |||
hasattr(trainer, 'resume_from_checkpoint') | |||
and trainer.checkpoint_connector.resume_checkpoint_path is not None | |||
and trainer._checkpoint_connector.resume_checkpoint_path is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo: ask PTL team for stable API for changing connectors
@@ -1,3 +1,4 @@ | |||
setuptools==59.5.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo: revert this when setuptools fixes itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a couple of comments/questions.
@@ -47,10 +47,11 @@ def main(cfg) -> None: | |||
exp_manager(trainer, cfg.exp_manager) | |||
|
|||
# update resume from checkpoint found by exp_manager | |||
resume_from_checkpoint = trainer.checkpoint_connector.resume_from_checkpoint_fit_path | |||
resume_from_checkpoint = trainer._checkpoint_connector.resume_from_checkpoint_fit_path | |||
# resume_from_checkpoint = uninject_model_parallel_rank(resume_from_checkpoint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could uncomment this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Misunderstood the wandb part.
What does this PR do ?
This PR will have fixes needed for PTL 1.6.0. The PR will first install PTL 1.6.0rc0 via the Jenkinsfile. Then when PTL 1.6.0 is on PYPI, PTL will be installed via .reinstall.sh. Do not merge this PR until PTL 1.6.0 is on PYPI.
Collection: Core
Changelog
Usage
# Add a code snippet demonstrating how to use this
Before your PR is "Ready for review"
Pre checks:
PR Type:
If you haven't finished some of the above items you can still open "Draft" PR.
Who can review?
Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.
Additional Information