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

[tune] Remove the URI parsing code #38697

Merged
merged 16 commits into from
Aug 24, 2023

Conversation

ericl
Copy link
Contributor

@ericl ericl commented Aug 21, 2023

Why are these changes needed?

  • Remove the URI parsing code for reconstructing output storage URIs
  • Provide "filesystem" attribute for ResultGrid, Result
  • Update tests

Related issue number

Closes #38568

@ericl ericl changed the title [WIP] Remove the URI parsing code [tune] Remove the URI parsing code Aug 22, 2023
Signed-off-by: Eric Liang <ekhliang@gmail.com>
Signed-off-by: Eric Liang <ekhliang@gmail.com>
Signed-off-by: Eric Liang <ekhliang@gmail.com>
Signed-off-by: Eric Liang <ekhliang@gmail.com>
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, looks much better now.

python/ray/tune/tune.py Outdated Show resolved Hide resolved
python/ray/tune/analysis/experiment_analysis.py Outdated Show resolved Hide resolved
python/ray/tune/analysis/experiment_analysis.py Outdated Show resolved Hide resolved
@@ -91,6 +93,15 @@ def experiment_path(self) -> str:
location (path on the head node)."""
return self._experiment_analysis.experiment_path

@property
def filesystem(self) -> pyarrow.fs.FileSystem:
Copy link
Contributor

Choose a reason for hiding this comment

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

keep this named storage_filesystem? fine either way.

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 it's best to be consistent with checkpoint, which uses filesystem as the attribute.

Signed-off-by: Eric Liang <ekhliang@gmail.com>
Signed-off-by: Eric Liang <ekhliang@gmail.com>
Signed-off-by: Eric Liang <ekhliang@gmail.com>
Signed-off-by: Eric Liang <ekhliang@gmail.com>
Signed-off-by: Eric Liang <ekhliang@gmail.com>
Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Eric Liang <ekhliang@gmail.com>
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@ericl ericl added Ray 2.7 tests-ok The tagger certifies test failures are unrelated and assumes personal liability. labels Aug 24, 2023
@ericl
Copy link
Contributor Author

ericl commented Aug 24, 2023

The failing test is flaking on master with the exact same error message.

@zhe-thoughts can you merge? This is needed to flip on the new persistence backend.

Copy link
Collaborator

@zhe-thoughts zhe-thoughts left a comment

Choose a reason for hiding this comment

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

Approving this, since it's part of Ray Train API cleanup

@zhe-thoughts zhe-thoughts merged commit aae9135 into ray-project:master Aug 24, 2023
66 of 73 checks passed
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
Signed-off-by: Eric Liang <ekhliang@gmail.com>
Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
Signed-off-by: Eric Liang <ekhliang@gmail.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
Labels
Ray 2.7 tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tune/train] Remove URI parsing of paths
5 participants