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] Add *path properties to Result and ResultGrid #33410

Merged
merged 22 commits into from
Mar 24, 2023
Merged

[air] Add *path properties to Result and ResultGrid #33410

merged 22 commits into from
Mar 24, 2023

Conversation

krfricke
Copy link
Contributor

@krfricke krfricke commented Mar 17, 2023 •
edited
Loading

Why are these changes needed?

Following #33370, this PR adds Result.path and ResultGrid.experiment_path to the respective classes. Further, we remove the public facing ExperimentAnalysis.local_path and ExperimentAnalysis.remote_path in favor of a unified ExperimentAnalysis.experiment_path.

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 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 :(

Kai Fricke added 7 commits March 16, 2023 19:43
Comments
7c72341 
Signed-off-by: Kai Fricke <kai@anyscale.com>
revert
ff87a60 
Signed-off-by: Kai Fricke <kai@anyscale.com>
revert
9c5088f 
Signed-off-by: Kai Fricke <kai@anyscale.com>
Add path property to result and result grid
c900cb7 
Signed-off-by: Kai Fricke <kai@anyscale.com>
Test
890a33e 
Signed-off-by: Kai Fricke <kai@anyscale.com>
Docs
bb3f958 
Signed-off-by: Kai Fricke <kai@anyscale.com>
Fix test
c1ee231 
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Mar 17, 2023
@richardliaw
Copy link
Contributor

Hey, did we end up going through #api-changes for this? Also, this doesn't seem to correspond with our discussion on the PRD..?

private properties
3064469 
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke removed the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Mar 19, 2023
Kai Fricke added 3 commits March 19, 2023 16:43
Merge remote-tracking branch 'upstream/master' into tune/results-prop…
57b32ec 
…erties
property
d726600 
Signed-off-by: Kai Fricke <kai@anyscale.com>
fix results grid test
7e8d896 
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Mar 20, 2023
experiment path
2901948 
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke removed the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Mar 22, 2023
Kai Fricke added 2 commits March 21, 2023 17:50
Update docstring
ce17625 
Signed-off-by: Kai Fricke <kai@anyscale.com>
more docstrings
430a7e7 
Signed-off-by: Kai Fricke <kai@anyscale.com>
ericl
ericl reviewed Mar 22, 2023
View reviewed changes
python/ray/air/checkpoint.py Outdated Show resolved Hide resolved
checkpoint docstring
efc6fe4 
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke requested a review from ericl March 22, 2023 17:26
@krfricke krfricke assigned richardliaw and unassigned justinvyu Mar 22, 2023
justinvyu
justinvyu reviewed Mar 22, 2023
View reviewed changes
Copy link
Contributor

@justinvyu justinvyu left a comment

Choose a reason for hiding this comment

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

Thanks! Just added a few questions:

python/ray/tune/tests/test_result_grid.py Show resolved Hide resolved
python/ray/air/checkpoint.py
@@ -246,6 +246,37 @@ def _metadata(self, metadata: _CheckpointMetadata):
for attr, value in metadata.checkpoint_state.items():
setattr(self, attr, value)

@property
def path(self) -> Optional[str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should checkpoint.uri get deprecated given this one?

Also, what if a user wants the local path for some reason when using cloud? Can only access through private property right now.

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 think we should keep URI to have counterparts to to_uri and from_uri for now. But I agree, we should probably see if we want to move to to_path and from_path instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For accessing the local path, if a checkpoint has a URI, the checkpoint object does not have a local path stored. There may be cached data somewhere on the local node, but it doesn't have to be (e.g. if the checkpoint was stored on a worker node). The user can use checkpoint.to_directory() to download the data to a target directory.

python/ray/air/checkpoint.py Show resolved Hide resolved
python/ray/air/checkpoint.py Show resolved Hide resolved
python/ray/tune/tests/test_result_grid.py Show resolved Hide resolved
python/ray/tune/result_grid.py Outdated Show resolved Hide resolved
Documentation
71cc4eb 
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke merged commit fe730c5 into ray-project:master Mar 24, 2023
@krfricke krfricke deleted the tune/results-properties branch March 24, 2023 06:51
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
@krfricke @elliottower
[air] Add *path properties to Result and ResultGrid (ray-projec…
53e551f 
…t#33410)

Following ray-project#33370, this PR adds `Result.path` and `ResultGrid.experiment_path` to the respective classes. Further, we remove the public facing `ExperimentAnalysis.local_path` and `ExperimentAnalysis.remote_path` in favor of a unified `ExperimentAnalysis.experiment_path`.

Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: elliottower <elliot@elliottower.com>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
@krfricke @ProjectsByJackHe
[air] Add *path properties to Result and ResultGrid (ray-projec…
6caf83e 
…t#33410)

Following ray-project#33370, this PR adds `Result.path` and `ResultGrid.experiment_path` to the respective classes. Further, we remove the public facing `ExperimentAnalysis.local_path` and `ExperimentAnalysis.remote_path` in favor of a unified `ExperimentAnalysis.experiment_path`.

Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Jack He <jackhe2345@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardliaw richardliaw richardliaw approved these changes

@matthewdeng matthewdeng Awaiting requested review from matthewdeng matthewdeng is a code owner

@xwjiang2010 xwjiang2010 Awaiting requested review from xwjiang2010

@ericl ericl Awaiting requested review from ericl

@justinvyu justinvyu Awaiting requested review from justinvyu justinvyu is a code owner

Assignees

@ericl ericl

@justinvyu justinvyu

@matthewdeng matthewdeng

@richardliaw richardliaw

@xwjiang2010 xwjiang2010

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@krfricke @richardliaw @ericl @justinvyu @matthewdeng @xwjiang2010