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] Move to new storage_path API in tests and examples #34263

Merged
merged 14 commits into from
Apr 13, 2023

Conversation

krfricke
Copy link
Contributor

@krfricke krfricke commented Apr 11, 2023

Why are these changes needed?

Following #33463, this PR updates our tests, examples, and docs to use the new storage_path API.

The only locations where we continue to use the local_dir statement are tests where we specify both a local dir and a remote dir. For these tests, we can move to an environment-variable based wrapper in the future.

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

Kai Fricke added 8 commits April 5, 2023 18:15
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
@xwjiang2010
Copy link
Contributor

Unit test looks good. Can we also run two release tests: 1. one tune scalability and 2. one tune cloud test

@krfricke
Copy link
Contributor Author

Yes, release tests are here: https://buildkite.com/ray-project/release-tests-pr/builds/34534#_

The worker fault tolerance test failure seems unrelated to the changes but I'll double check.

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.

Looks good.

I do have a question about a SyncConfig.upload_dir remainder that is causing some unintended effects:

Experiment checkpoint upload logic currently checks for upload_dir in order to determine whether or not to ignore uploading checkpoint dirs. We should just check for self._remote_checkpoint_dir and remove the need for sync_config in this case:

if bool(self._sync_config.upload_dir):

Should we fix this in the current PR? Or is there another PR queued up already?

Kai Fricke added 4 commits April 12, 2023 12:33
Signed-off-by: Kai Fricke <kai@anyscale.com>
…-tests-examples

# Conflicts:
#	doc/source/tune/doc_code/fault_tolerance.py
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke
Copy link
Contributor Author

The test failure seems unrelated - it's a segfault that seems to come from a different source. It also seems to be a flaky test. I've left some debug prints in the test for the next time this failure occurs, but assuming the unit tests pass, we can merge this PR.

Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke
Copy link
Contributor Author

Looks good.

I do have a question about a SyncConfig.upload_dir remainder that is causing some unintended effects:

Experiment checkpoint upload logic currently checks for upload_dir in order to determine whether or not to ignore uploading checkpoint dirs. We should just check for self._remote_checkpoint_dir and remove the need for sync_config in this case:

if bool(self._sync_config.upload_dir):

Should we fix this in the current PR? Or is there another PR queued up already?

Thanks! Overlooked this one. Fixed

@krfricke krfricke requested a review from justinvyu April 13, 2023 12:31
Copy link
Contributor

@richardliaw richardliaw left a comment

Choose a reason for hiding this comment

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

stamp docs

Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke merged commit d6071a6 into ray-project:master Apr 13, 2023
@krfricke krfricke deleted the air/local-storage-tests-examples branch April 13, 2023 20:43
vitsai pushed a commit to vitsai/ray that referenced this pull request Apr 17, 2023
…#34263)

Following ray-project#33463, this PR updates our tests, examples, and docs to use the new `storage_path` API.

The only locations where we continue to use the `local_dir` statement are tests where we specify both a local dir and a remote dir. For these tests, we can move to an environment-variable based wrapper in the future.

Signed-off-by: Kai Fricke <kai@anyscale.com>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
…#34263)

Following ray-project#33463, this PR updates our tests, examples, and docs to use the new `storage_path` API.

The only locations where we continue to use the `local_dir` statement are tests where we specify both a local dir and a remote dir. For these tests, we can move to an environment-variable based wrapper in the future.

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
…#34263)

Following ray-project#33463, this PR updates our tests, examples, and docs to use the new `storage_path` API.

The only locations where we continue to use the `local_dir` statement are tests where we specify both a local dir and a remote dir. For these tests, we can move to an environment-variable based wrapper in the future.

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants