Skip to content

[build_base] [Tune] Add more comprehensive support for remote upload_dir w/ endpoint and params - #32479

Merged
krfricke merged 18 commits into
ray-project:masterfrom
justinvyu:tune/remote_checkpoint_dir_with_endpoint
Feb 15, 2023
Merged

[build_base] [Tune] Add more comprehensive support for remote upload_dir w/ endpoint and params#32479
krfricke merged 18 commits into
ray-project:masterfrom
justinvyu:tune/remote_checkpoint_dir_with_endpoint

Conversation

@justinvyu

@justinvyu justinvyu commented Feb 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Why are these changes needed?

Problem

Currently, URI handling with parameters is done in multiple places in different ways (using urllib.parse or splitting by '?' directly). In some places, it's not done at all, which causes errors when performing cloud checkpointing. In particular, Trial.remote_checkpoint_dir and Trainable._storage_path do not handle URI path appends correctly when URL params are present.

Example:

SyncConfig(upload_dir="s3://bucket?region=us-west-2&endpoint_override=http://localhost:5002")

Solution

This PR introduces a URI utility class that appends/gets the parent/name of a URI. All of the URI handling now goes through this utility, rather than doing os.path.join/manually parsing the string.

This PR also improves the moto mock s3 simulate_storage helper, and introduces a fixture in test_syncer and example usage in an end to end Tune run that saves to the cloud.

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

@justinvyu
Add URI utils
4f30451 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Fix remote checkpoint dir property
eed3b5e 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Add name property
1c6911a 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Use util in tuner restore
9b13213 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Add to experiment remote ckpt dir
d55360e 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Fix lint
9f3c5f0 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Add test for trial remote ckpt dir
ae4d579 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Fix remote storage path util to use URI path append
bcd1694 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Add unit test + improve simulate_storage to use built-in ThreadedMoto…
90c5f5f 
…Server

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Convert other test using mock s3
a9e1410 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Fix lint
1a52673 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Merge branch 'master' of https://github.com/ray-project/ray into tune…
b2eb115 
…/remote_checkpoint_dir_with_endpoint
@justinvyu
Add docstring to simulate_storage
a6bf88d 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Add ray shutdown to syncer test
cc1bdae 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Merge branch 'master' of https://github.com/ray-project/ray into tune…
5a67239 
…/remote_checkpoint_dir_with_endpoint
Yard1
Yard1 approved these changes Feb 13, 2023
View reviewed changes

@Yard1 Yard1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Really nice!

@justinvyu justinvyu changed the title [Tune] Add more comprehensive support for remote upload_dir w/ endpoint and params [build_base] [Tune] Add more comprehensive support for remote upload_dir w/ endpoint and params Feb 13, 2023
@justinvyu
Merge branch 'master' of https://github.com/ray-project/ray into tune…
3033c8d 
…/remote_checkpoint_dir_with_endpoint
@justinvyu
Add back default cloud root
8757137 
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Merge branch 'master' of https://github.com/ray-project/ray into tune…
c7aa906 
…/remote_checkpoint_dir_with_endpoint
krfricke
krfricke approved these changes Feb 14, 2023
View reviewed changes

@krfricke krfricke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome! Let's run cloud tests

@krfricke
krfricke merged commit 2f2080f into ray-project:master Feb 15, 2023
justinvyu added a commit to justinvyu/ray that referenced this pull request Feb 15, 2023
@justinvyu
Revert "[build_base] [Tune] Add more comprehensive support for remote…
6d928e9 
… `upload_dir` w/ endpoint and params (ray-project#32479)"

This reverts commit 2f2080f.
xwjiang2010 pushed a commit that referenced this pull request Feb 15, 2023
@justinvyu
Revert "[build_base] [Tune] Add more comprehensive support for remote…
350fb13 
… `upload_dir` w/ endpoint and params (#32479)" (#32571)

This reverts commit 2f2080f.
justinvyu added a commit to justinvyu/ray that referenced this pull request Feb 15, 2023
@justinvyu
Revert "Revert "[build_base] [Tune] Add more comprehensive support fo…
f0af5c9 
…r remote `upload_dir` w/ endpoint and params (ray-project#32479)" (ray-project#32571)"

This reverts commit 350fb13.
krfricke pushed a commit that referenced this pull request Feb 16, 2023
@justinvyu
[Tune] Unrevert "Add more comprehensive support for remote_checkpoint…
abdb7c7 
…_dir w/ url params (#32479)" (#32576)

This reverts the revert made here #32571.

This PR fixes two suites of tests that were failing:
1. workflows tests were failing due to the pinned `moto` version being too old.
2. `test_syncer` errors were caused by reusing the same bucket name between tests.

This PR also fixes the `restore` path for URIs with params, which was missing from the previous PR. The e2e sync test has been updated to also test restoration.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
@justinvyu @edoakes
[build_base] [Tune] Add more comprehensive support for remote `upload…
9931d00 
…_dir` w/ endpoint and params (ray-project#32479)

Currently, URI handling with parameters is done in multiple places in different ways (using `urllib.parse` or splitting by `'?'` directly). In some places, it's not done at all, which **causes errors when performing cloud checkpointing.** In particular, `Trial.remote_checkpoint_dir` and `Trainable._storage_path` do not handle URI path appends correctly when URL params are present.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
@justinvyu @edoakes
Revert "[build_base] [Tune] Add more comprehensive support for remote…
dcd0550 
… `upload_dir` w/ endpoint and params (ray-project#32479)" (ray-project#32571)

This reverts commit 2f2080f.

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
@justinvyu @edoakes
[Tune] Unrevert "Add more comprehensive support for remote_checkpoint…
65a4fbd 
…_dir w/ url params (ray-project#32479)" (ray-project#32576)

This reverts the revert made here ray-project#32571.

This PR fixes two suites of tests that were failing:
1. workflows tests were failing due to the pinned `moto` version being too old.
2. `test_syncer` errors were caused by reusing the same bucket name between tests.

This PR also fixes the `restore` path for URIs with params, which was missing from the previous PR. The e2e sync test has been updated to also test restoration.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
peytondmurray pushed a commit to peytondmurray/ray that referenced this pull request Mar 22, 2023
@justinvyu @peytondmurray
Revert "[build_base] [Tune] Add more comprehensive support for remote…
cfa011d 
… `upload_dir` w/ endpoint and params (ray-project#32479)" (ray-project#32571)

This reverts commit 2f2080f.
peytondmurray pushed a commit to peytondmurray/ray that referenced this pull request Mar 22, 2023
@justinvyu @peytondmurray
[Tune] Unrevert "Add more comprehensive support for remote_checkpoint…
af52b5f 
…_dir w/ url params (ray-project#32479)" (ray-project#32576)

This reverts the revert made here ray-project#32571.

This PR fixes two suites of tests that were failing:
1. workflows tests were failing due to the pinned `moto` version being too old.
2. `test_syncer` errors were caused by reusing the same bucket name between tests.

This PR also fixes the `restore` path for URIs with params, which was missing from the previous PR. The e2e sync test has been updated to also test restoration.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
@justinvyu @elliottower
[build_base] [Tune] Add more comprehensive support for remote `upload…
f1d5881 
…_dir` w/ endpoint and params (ray-project#32479)

Currently, URI handling with parameters is done in multiple places in different ways (using `urllib.parse` or splitting by `'?'` directly). In some places, it's not done at all, which **causes errors when performing cloud checkpointing.** In particular, `Trial.remote_checkpoint_dir` and `Trainable._storage_path` do not handle URI path appends correctly when URL params are present.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: elliottower <elliot@elliottower.com>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
@justinvyu @elliottower
Revert "[build_base] [Tune] Add more comprehensive support for remote…
fe5c2f3 
… `upload_dir` w/ endpoint and params (ray-project#32479)" (ray-project#32571)

This reverts commit 2f2080f.

Signed-off-by: elliottower <elliot@elliottower.com>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
@justinvyu @elliottower
[Tune] Unrevert "Add more comprehensive support for remote_checkpoint…
2377b34 
…_dir w/ url params (ray-project#32479)" (ray-project#32576)

This reverts the revert made here ray-project#32571.

This PR fixes two suites of tests that were failing:
1. workflows tests were failing due to the pinned `moto` version being too old.
2. `test_syncer` errors were caused by reusing the same bucket name between tests.

This PR also fixes the `restore` path for URIs with params, which was missing from the previous PR. The e2e sync test has been updated to also test restoration.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: elliottower <elliot@elliottower.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Yard1 Yard1 Yard1 approved these changes
@krfricke krfricke krfricke approved these changes
@xwjiang2010 xwjiang2010 Awaiting requested review from xwjiang2010

Assignees

@Yard1 Yard1

@krfricke krfricke

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@justinvyu @Yard1 @krfricke