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

[air] pyarrow.fs persistence (6/n): Fix Trial + Experiment paths to use the StorageContext #38057

Conversation

justinvyu
Copy link
Contributor

@justinvyu justinvyu commented Aug 3, 2023

Why are these changes needed?

This PR cleans up the path handling in Experiment and Trial to use the StorageContext, and marks all the legacy attributes that can soon be removed.

This fixes an issue when specifying a custom storage_filesystem:

Before:

RunConfig(storage_path="mock/a/b/c", storage_filesystem=CustomFileSystem())

# Experiment / Trial path handling would expand `mock/a/b/c` -> `{os.getcwd()}/mock/a/b/c`
# Basically, we would be treating this storage path as a relative path in certain cases.

result_grid = tuner.fit()

# these use the incorrect expanded abs path
result_grid.experiment_path   #  -> os.getcwd/mock/a/b/c/exp_name
result_grid[0].path   #  -> os.getcwd/mock/a/b/c/exp_name/trial_name

After:

# Paths stay relative
result_grid.experiment_path   #  -> mock/a/b/c/exp_name
result_grid[0].path   #  -> mock/a/b/c/exp_name/trial_name

Q: If a user passes in a relative directory without a custom storage fs (aka a local path), should we expand it to an absolute path?

Other comments

There are now only 2 path concepts: fs_path vs local_path. storage_prefix contains the stripped URI prefix information that can be used by consumers (ex: Result) in order to reconstruct a URI similar to storage_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 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 :(

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

Update to use the new checkpoint id attribute

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

Add todo comment to remove legacy path

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

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

Fix lint

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

Fix lint

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
…persistence/storage_context_to_worker_temp
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
…persistence/new_checkpoint

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

Fix lint for session.py

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

Fix lint for storage.py

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@ericl
Copy link
Contributor

ericl commented Aug 3, 2023

Should rebase to clean up the diff.

…persistence/fix_custom_fs_path_expansion

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
…persistence/fix_custom_fs_path_expansion
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
…persistence/fix_custom_fs_path_expansion
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu justinvyu requested a review from ericl August 4, 2023 19:55
@justinvyu justinvyu removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 4, 2023
@ericl
Copy link
Contributor

ericl commented Aug 4, 2023

Cool, this looks much cleaner.

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 4, 2023
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu justinvyu added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Aug 5, 2023
@justinvyu
Copy link
Contributor Author

@ericl Tests look ok for merge now

@ericl ericl merged commit 470c10b into ray-project:master Aug 5, 2023
77 of 82 checks passed
@justinvyu justinvyu deleted the air/persistence/fix_custom_fs_path_expansion branch August 5, 2023 18:34
NripeshN pushed a commit to NripeshN/ray that referenced this pull request Aug 15, 2023
…s to use the `StorageContext` (ray-project#38057)

Signed-off-by: NripeshN <nn2012@hw.ac.uk>
harborn pushed a commit to harborn/ray that referenced this pull request Aug 17, 2023
…s to use the `StorageContext` (ray-project#38057)

Signed-off-by: harborn <gangsheng.wu@intel.com>
harborn pushed a commit to harborn/ray that referenced this pull request Aug 17, 2023
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
…s to use the `StorageContext` (ray-project#38057)

Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
…s to use the `StorageContext` (ray-project#38057)

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
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.

None yet

2 participants