Mark CheckpointConnector as protected #11550

krishnakalyan3 · 2022-01-20T13:35:34Z

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

rohitgr7 · 2022-01-20T13:39:44Z

hey @krishnakalyan3 !
Signal connector is already done here: #11513
would you mind rebasing your branch with master?

also, you mean do to CheckpointConnector?

krishnakalyan3 · 2022-01-20T13:43:46Z

Wow looks like I am super slow. You are right I made a mistake while naming the branch.

rohitgr7 · 2022-01-20T13:47:48Z

Wow looks like I am super slow. You are right I made a mistake while naming the branch.

you can still continue here. branch name looks correct :)

krishnakalyan3 · 2022-01-20T15:06:28Z

Fixes #11482

krishnakalyan3 · 2022-01-20T15:25:14Z

@rohitgr7 should the depreciated files below be changed to protected?

./tests/deprecated_api/test_remove_2-0.py:    assert trainer.checkpoint_connector.resume_checkpoint_path is None
./tests/deprecated_api/test_remove_2-0.py:    assert trainer.checkpoint_connector.resume_from_checkpoint_fit_path == ckpt_path
./tests/deprecated_api/test_remove_2-0.py:    assert trainer.checkpoint_connector.resume_checkpoint_path is None
./tests/deprecated_api/test_remove_2-0.py:    assert trainer.checkpoint_connector.resume_from_checkpoint_fit_path == ckpt_path
./tests/deprecated_api/test_remove_2-0.py:    assert trainer.checkpoint_connector.resume_checkpoint_path is None
./tests/deprecated_api/test_remove_2-0.py:    assert trainer.checkpoint_connector.resume_from_checkpoint_fit_path is None
./tests/deprecated_api/test_remove_2-0.py:    assert trainer.checkpoint_connector.resume_checkpoint_path is None
./tests/deprecated_api/test_remove_2-0.py:    assert trainer.checkpoint_connector.resume_from_checkpoint_fit_path is None
./tests/deprecated_api/test_remove_2-0.py:    assert trainer.checkpoint_connector.resume_checkpoint_path is None
./tests/deprecated_api/test_remove_2-0.py:    assert trainer.checkpoint_connector.resume_from_checkpoint_fit_path is None

rohitgr7 · 2022-01-20T15:28:24Z

@krishnakalyan3 yes!
else it will fail eventually since we are not targeting any BC for trainer.checkpoint_connector.

krishnakalyan3 · 2022-01-20T15:34:22Z

@rohitgr7 Looks like I have made the changes to all files, could you please review it?.

rohitgr7

Thank you for your contribution 😃

krishnakalyan3 · 2022-01-20T20:59:38Z

Thank you for the reviews.

codecov · 2022-01-20T21:18:03Z

Codecov Report

Merging #11550 (9ea4115) into master (92c22ae) will decrease coverage by 4%.
The diff coverage is 92%.

❗ Current head 9ea4115 differs from pull request most recent head 975ab63. Consider uploading reports for the commit 975ab63 to get more accurate results

@@           Coverage Diff            @@
##           master   #11550    +/-   ##
========================================
- Coverage      92%      88%    -4%     
========================================
  Files         194      194            
  Lines       16947    16944     -3     
========================================
- Hits        15562    14945   -617     
- Misses       1385     1999   +614

tchaton

LGTM !

daniellepintz · 2022-01-21T19:39:59Z

The GPUs test has been queued since yesterday, can someone rerun it?

rohitgr7 · 2022-01-22T20:39:31Z

The GPUs test has been queued since yesterday, can someone rerun it?

@daniellepintz there is some issue on the Azure side. All the PRs are in the queue.

krishnakalyan3 · 2022-01-26T14:44:54Z

Looks like the GPU tests are still pending?

akihironitta · 2022-01-26T19:43:54Z

@krishnakalyan3 Yes. The tracking issue is #11634.

akihironitta

Although deprecation is not needed, shouldn't we add an entry to CHANGELOG.md similarly to #9779?

Borda

changelog entry as it could be also seen as breaking change...?

krishnakalyan3 requested review from awaelchli, Borda, carmocca, justusschock, kaushikb11, rohitgr7, SeanNaren, tchaton and williamFalcon as code owners January 20, 2022 13:35

krishnakalyan3 changed the title ~~mark SignalConnector as protected~~ Mark SignalConnector as protected Jan 20, 2022

krishnakalyan3 closed this Jan 20, 2022

krishnakalyan3 reopened this Jan 20, 2022

checkpoint_connector private

bc1de18

krishnakalyan3 added 2 commits January 20, 2022 16:09

checkpoint_connector tests private

443fda2

misc changes to checkpoint_connector

4d68314

krishnakalyan3 requested a review from SkafteNicki as a code owner January 20, 2022 15:15

rohitgr7 changed the title ~~Mark SignalConnector as protected~~ Mark CheckpointConnector as protected Jan 20, 2022

add more test as protected

3fdb1ff

krishnakalyan3 added 3 commits January 20, 2022 16:29

checkpoint_connector test update

5021f29

update tests for checkpoint_connector as private

3a9bfa9

changes to the test_trainer file

3544c5e

rohitgr7 added design Includes a design discussion refactor labels Jan 20, 2022

rohitgr7 added this to the 1.6 milestone Jan 20, 2022

rohitgr7 approved these changes Jan 20, 2022

View reviewed changes

ananthsub approved these changes Jan 20, 2022

View reviewed changes

mergify bot added the ready PRs ready to be merged label Jan 20, 2022

carmocca approved these changes Jan 20, 2022

View reviewed changes

carmocca removed the design Includes a design discussion label Jan 20, 2022

rohitgr7 enabled auto-merge (squash) January 20, 2022 17:56

ananthsub approved these changes Jan 20, 2022

View reviewed changes

Merge branch 'master' into check-point-protected

9ea4115

krishnakalyan3 closed this Jan 20, 2022

auto-merge was automatically disabled January 20, 2022 20:59
Pull request was closed

ananthsub reopened this Jan 20, 2022

carmocca enabled auto-merge (squash) January 21, 2022 01:43

tchaton approved these changes Jan 21, 2022

View reviewed changes

awaelchli approved these changes Jan 21, 2022

View reviewed changes

akihironitta approved these changes Jan 26, 2022

View reviewed changes

krishnakalyan3 mentioned this pull request Feb 2, 2022

Deprecate on_before_accelerator_backend_setup callback hook #11655

Merged

12 tasks

Borda approved these changes Feb 2, 2022

View reviewed changes

Merge branch 'master' into check-point-protected

975ab63

Borda reviewed Feb 2, 2022

View reviewed changes

carmocca added 2 commits February 2, 2022 23:49

Merge branch 'master' into check-point-protected

84753b7

Update CHANGELOG

98ca31b

carmocca merged commit 6586dd2 into Lightning-AI:master Feb 3, 2022

Mark CheckpointConnector as protected #11550

Mark CheckpointConnector as protected #11550

Uh oh!

Conversation

krishnakalyan3 commented Jan 20, 2022 • edited by carmocca Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

Uh oh!

rohitgr7 commented Jan 20, 2022

Uh oh!

krishnakalyan3 commented Jan 20, 2022

Uh oh!

rohitgr7 commented Jan 20, 2022

Uh oh!

krishnakalyan3 commented Jan 20, 2022

Uh oh!

krishnakalyan3 commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rohitgr7 commented Jan 20, 2022

Uh oh!

krishnakalyan3 commented Jan 20, 2022

Uh oh!

rohitgr7 left a comment

Choose a reason for hiding this comment

Uh oh!

krishnakalyan3 commented Jan 20, 2022

Uh oh!

codecov bot commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tchaton left a comment

Choose a reason for hiding this comment

Uh oh!

daniellepintz commented Jan 21, 2022

Uh oh!

rohitgr7 commented Jan 22, 2022

Uh oh!

krishnakalyan3 commented Jan 26, 2022

Uh oh!

akihironitta commented Jan 26, 2022

Uh oh!

akihironitta left a comment

Choose a reason for hiding this comment

Uh oh!

Borda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krishnakalyan3 commented Jan 20, 2022 •

edited by carmocca

Loading

krishnakalyan3 commented Jan 20, 2022 •

edited

Loading

codecov bot commented Jan 20, 2022 •

edited

Loading