[Tune] Make ResultGrid return cloud checkpoints #31437
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: Antoni Baum <[email protected]>
justinvyu
requested changes
Jan 4, 2023
Signed-off-by: Antoni Baum <[email protected]>
|
@justinvyu different approach, PTAL! |
Signed-off-by: Antoni Baum <[email protected]>
Signed-off-by: Antoni Baum <[email protected]>
justinvyu
approved these changes
Jan 5, 2023
| @@ -185,6 +185,15 @@ def get_checkpoints_paths(logdir): | |||
| ) | |||
| return chkpt_df | |||
|
|
|||
| @staticmethod | |||
| def get_remote_storage_path( | |||
| local_path: str, logdir: str, remote_checkpoint_dir: str | |||
There was a problem hiding this comment.
Can we rename remote_checkpoint_dir to remote_logdir? Seems like two different concepts with the current naming but one is just the cloud version of the other.
There was a problem hiding this comment.
Wanted to use the same names as in Trial.
Co-authored-by: Justin Yu <[email protected]> Signed-off-by: Antoni Baum <[email protected]>
Signed-off-by: Antoni Baum <[email protected]>
Signed-off-by: Antoni Baum <[email protected]>
andreapiso
pushed a commit
to andreapiso/ray
that referenced
this pull request
Jan 22, 2023
`Checkpoint`s returned by `ResultGrid` will never point to a cloud URI even if cloud syncing is enabled. This PR fixes this by saving information necessary to turn a local path into a remote path in `_TrackedCheckpoint`, and applying it during conversion to an AIR checkpoint. An alternate approach would be to save `remote_upload_dir` and `logdir` in `_TrackedCheckpoint` instead of a function, or for `Trainable.save` to return a tuple of `(local_path, remote_path)` if saved to cloud. Signed-off-by: Antoni Baum <[email protected]> Co-authored-by: Justin Yu <[email protected]> Signed-off-by: Andrea Pisoni <[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: Antoni Baum [email protected]
Why are these changes needed?
Checkpoints returned byResultGridwill never point to a cloud URI even if cloud syncing is enabled. This PR fixes this by saving information necessary to turn a local path into a remote path in_TrackedCheckpoint, and applying it during conversion to an AIR checkpoint.An alternate approach would be to save
remote_upload_dirandlogdirin_TrackedCheckpointinstead of a function, or forTrainable.saveto return a tuple of(local_path, remote_path)if saved to cloud.Related issue number
Closes #31001
Closes #28492
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.