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.

Sign up for GitHub

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

Jump to bottom

[air] Remove ray.train.checkpoint namespace #37991

Merged
merged 6 commits into from
Aug 2, 2023
Merged

[air] Remove ray.train.checkpoint namespace #37991

merged 6 commits into from
Aug 2, 2023

Conversation

justinvyu
Copy link
Contributor

@justinvyu justinvyu commented Aug 1, 2023 •
edited
Loading

Why are these changes needed?

This PR makes a few fixes:

  • Renames ray.train.checkpoint -> ray.train._checkpoint to remove the confusing ray.train.checkpoint namespace. The alias ray.train.Checkpoint needs to be assigned to ray.train._checkpoint.Checkpoint by the time of 2.7 release.
  • Fixes semgrep CI failing due to a code-block that should be a testcode.
  • Fixes train.get_checkpoint usage in a test that changed from a recent PR.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@justinvyu
Convert code example to testcode
7273a54 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
bveeramani
bveeramani approved these changes Aug 1, 2023
View reviewed changes
Copy link
Member

@bveeramani bveeramani left a comment

Choose a reason for hiding this comment

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

Thanks!

pcmoritz
pcmoritz reviewed Aug 1, 2023
View reviewed changes
python/ray/train/checkpoint.py Outdated
from pathlib import Path
import tempfile

from ray.train.checkpoint import Checkpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't do this, see #37925 (comment)

Copy link
Contributor Author

@justinvyu justinvyu Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm currently forced to do this, since 2 "Checkpoint" classes exist at the moment:

from ray.air import Checkpoint  # old one, used in current examples, currently being phased out
from ray.train._checkpoint import Checkpoint  # new checkpoint interface

__all__ = ["Checkpoint"]  # These 2 checkpoints are colliding -- I can't expose both the same way.

By 2.7, I will definitely make the switch for ray.train.Checkpoint to point to ray.train._checkpoint.Checkpoint.
But, we're not 100% ready to make the switch just yet (it will involve a bunch of tests to change, and the new Checkpoint is not fully supported yet by other internal components).

For now, I can add a TODO in this docstring to update it to ray.train.Checkpoint when the switch has happened. How does that sound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also happy to pre-emptively rename this to _checkpoint.py so that the ray.train.checkpoint namespace is no longer a thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, why don't you rename this to _checkpoint while this is still brewing to avoid confusion? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! PTAL

@justinvyu justinvyu changed the title [air] Convert ray.train.checkpoint code example to testcode [air] Remove ray.train.checkpoint namespace Aug 2, 2023
@justinvyu justinvyu requested a review from pcmoritz August 2, 2023 18:39
@justinvyu justinvyu added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Aug 2, 2023
@bveeramani bveeramani merged commit d60298b into ray-project:master Aug 2, 2023
49 of 55 checks passed
@justinvyu justinvyu deleted the fix_semgrep branch August 2, 2023 21:18
justinvyu added a commit to justinvyu/ray that referenced this pull request Aug 2, 2023
@justinvyu
[air] Remove ray.train.checkpoint namespace (ray-project#37991)
bc4c300 
* Convert code example to testcode

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Rename ray.train.checkpoint -> ray.train._checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix checkpoint -> _checkpoint for imports

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix train.get_context().get_checkpoint -> train.get_checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

---------

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
NripeshN pushed a commit to NripeshN/ray that referenced this pull request Aug 15, 2023
@justinvyu @NripeshN
[air] Remove ray.train.checkpoint namespace (ray-project#37991)
c541d37 
* Convert code example to testcode

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Rename ray.train.checkpoint -> ray.train._checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix checkpoint -> _checkpoint for imports

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix train.get_context().get_checkpoint -> train.get_checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

---------

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: NripeshN <nn2012@hw.ac.uk>
harborn pushed a commit to harborn/ray that referenced this pull request Aug 17, 2023
@justinvyu @harborn
[air] Remove ray.train.checkpoint namespace (ray-project#37991)
037ae83 
* Convert code example to testcode

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Rename ray.train.checkpoint -> ray.train._checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix checkpoint -> _checkpoint for imports

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix train.get_context().get_checkpoint -> train.get_checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

---------

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: harborn <gangsheng.wu@intel.com>
harborn pushed a commit to harborn/ray that referenced this pull request Aug 17, 2023
@justinvyu @harborn
[air] Remove ray.train.checkpoint namespace (ray-project#37991)
9f27858 
* Convert code example to testcode

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Rename ray.train.checkpoint -> ray.train._checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix checkpoint -> _checkpoint for imports

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix train.get_context().get_checkpoint -> train.get_checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

---------

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
@justinvyu @arvind-chandra
[air] Remove ray.train.checkpoint namespace (ray-project#37991)
d54c690 
* Convert code example to testcode

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Rename ray.train.checkpoint -> ray.train._checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix checkpoint -> _checkpoint for imports

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix train.get_context().get_checkpoint -> train.get_checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

---------

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
@justinvyu
[air] Remove ray.train.checkpoint namespace (ray-project#37991)
6e34c70 
* Convert code example to testcode

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Rename ray.train.checkpoint -> ray.train._checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix checkpoint -> _checkpoint for imports

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

* Fix train.get_context().get_checkpoint -> train.get_checkpoint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

---------

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Victor <vctr.y.m@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcmoritz pcmoritz pcmoritz approved these changes

@bveeramani bveeramani bveeramani approved these changes

Assignees

@bveeramani bveeramani

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@justinvyu @pcmoritz @bveeramani