[tune] Only sync down from cloud if needed #26725
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Kai Fricke <[email protected]>
6 tasks
added 5 commits
July 20, 2022 18:59
Signed-off-by: Kai Fricke <[email protected]>
This reverts commit 005b187.
Signed-off-by: Kai Fricke <[email protected]>
Signed-off-by: Kai Fricke <[email protected]>
xwjiang2010
reviewed
Jul 25, 2022
| @@ -487,7 +490,8 @@ def save(self, checkpoint_dir: Optional[str] = None) -> str: | |||
| TrainableUtil.write_metadata(checkpoint_dir, metadata) | |||
|
|
|||
| # Maybe sync to cloud | |||
| self._maybe_save_to_cloud(checkpoint_dir) | |||
| if not prevent_upload: | |||
There was a problem hiding this comment.
How about syncing to driver?
I recall seeing these tmp folders on driver...
There was a problem hiding this comment.
As discussed, out of scope for this PR :-)
There was a problem hiding this comment.
(I'll address that in a different PR - this PR actually makes PBT fail while syncing to driver is not great but at least the experiment will continue to run).
xwjiang2010
approved these changes
Jul 25, 2022
|
let’s revisit this whole pbt/syncing business after the release push. Ideally I would like to also add a e2e test using the user's repro script, if it makes sense for us. |
Rohan138
pushed a commit
to Rohan138/ray
that referenced
this pull request
Jul 28, 2022
Currently, trainables will try to sync up/down temporary checkpoints from cloud storage, leading to errors. These erros come up e.g. with PBT, which heavily uses saving/restoring from objects. Instead, we should not sync these temporary checkpoints up at all, and we should generally not sync down if a local checkpoint directory exists, which will prevent us also from trying to sync down non-existent temporary checkpoint directories. See ray-project#26714 Signed-off-by: Kai Fricke <[email protected]> Signed-off-by: Rohan138 <[email protected]>
4 tasks
Stefan-1313
pushed a commit
to Stefan-1313/ray_mod
that referenced
this pull request
Aug 18, 2022
Currently, trainables will try to sync up/down temporary checkpoints from cloud storage, leading to errors. These erros come up e.g. with PBT, which heavily uses saving/restoring from objects. Instead, we should not sync these temporary checkpoints up at all, and we should generally not sync down if a local checkpoint directory exists, which will prevent us also from trying to sync down non-existent temporary checkpoint directories. See ray-project#26714 Signed-off-by: Kai Fricke <[email protected]> Signed-off-by: Stefan van der Kleij <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Kai Fricke [email protected]
Why are these changes needed?
Currently, trainables will try to sync up/down temporary checkpoints from cloud storage, leading to errors. These erros come up e.g. with PBT, which heavily uses saving/restoring from objects.
Instead, we should not sync these temporary checkpoints up at all, and we should generally not sync down if a local checkpoint directory exists, which will prevent us also from trying to sync down non-existent temporary checkpoint directories.
See #26714
Related issue number
Closes #26714
Related PR for 1.13.1: #26717
Checks
scripts/format.shto lint the changes in this PR.