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.

Sign up for GitHub

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

Jump to bottom

[tune/train] Get rid of downsyncing path and download just the result.json / result.csv on result #38598

Merged
merged 16 commits into from
Aug 21, 2023
Merged

[tune/train] Get rid of downsyncing path and download just the result.json / result.csv on result #38598

merged 16 commits into from
Aug 21, 2023

Conversation

ericl
Copy link
Contributor

@ericl ericl commented Aug 18, 2023 •
edited
Loading

Why are these changes needed?

  • Remove usage of syncer.sync_down() for the results.json/experiment state files. This had the unfortunate side effect of sometimes copying large trial artifacts to the driver.
  • Instead, explicitly copy down the files we need to resume. This includes the experiment_state.json and basic-variant-state.json files, as well as the result.json and progress.csv files for each trial dir. The download of the result files is deferred until trial initialization.
  • Tested manually --- in principle, should be covered by existing tests, but please advise otherwise.

Related issue number

Closes #38576

@ericl
fix sync
606b6f1 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@ericl
wip
15f693c 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@ericl
comment
70fa5a8 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@ericl
linte
905d773 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@ericl ericl changed the title [WIP] Get rid of downsyncing path and download just the result.json / result.csv on result [tnue/train] Get rid of downsyncing path and download just the result.json / result.csv on result Aug 18, 2023
@ericl ericl changed the title [tnue/train] Get rid of downsyncing path and download just the result.json / result.csv on result [tune/train] Get rid of downsyncing path and download just the result.json / result.csv on result Aug 18, 2023
@ericl ericl marked this pull request as ready for review August 18, 2023 22:36
justinvyu
justinvyu requested changes Aug 18, 2023
View reviewed changes
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.

Left some comments. The callback downloads look good.

python/ray/tune/logger/json.py Show resolved Hide resolved
python/ray/tune/execution/experiment_state.py Show resolved Hide resolved
python/ray/tune/execution/experiment_state.py Outdated
f"matching {match_prefix}.*{match_suffix}"
)

restore_latest_match("basic-variant-state", ".json")
Copy link
Contributor

@justinvyu justinvyu Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The searcher state file is a bit more annoying. We only have this searcher.restore_from_dir API, so probably we should just download to have minimal API changes.

The filename is not always basic_variant depending on the searcher type.

Glob self._search_alg.CKPT_FILE_TMPL.format("*") instead.

And if _search_alg is a SearchGenerator, you need to download _search_alg.searcher.CKPT_FILE_TMPL as well...

Or, maybe let's just download all jsons in there as a hack for 2.7 😄
Actually, it's a mix of pkls and jsons. Maybe just convert it all to pkl and download all pkls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh geez.. Yeah maybe let's just download all the *.json *.pkl at the root for now.

python/ray/tune/execution/experiment_state.py Show resolved Hide resolved
python/ray/tune/syncer.py Outdated Show resolved Hide resolved
python/ray/tune/logger/logger.py Outdated Show resolved Hide resolved
@ericl
change it to a todo
29c2327 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
ericl and others added 8 commits August 18, 2023 16:20
@ericl
revert
9a27a6e 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@ericl
make debug
49ff7de 
Signed-off-by: Eric Liang <ekhliang@gmail.com>
@justinvyu
fix last_checkpoint_result typehint
7df52fb 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
small misc fixes
177a520 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
fix optional trial.checkpoint
897ecb2 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
don't write headers again in csv on restore
4b7af0e 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
test restoring after deleting the local experiment dir
aa684c0 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
sync down only if the trial has a checkpoint to restore from
57c8edb 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
Copy link
Contributor

I tested it out and it seems to work (I just try restoring after deleting the local cache dir in the test case). Updated the PR with logic to skip downloading if a trial has no checkpoint. PTAL, I think it's good to go assuming that tests pass.

@justinvyu
Merge branch 'master' of https://github.com/ray-project/ray into air/…
1202858 
…persistence/no_sync_down
@justinvyu
fix trial stub missing ckpt
62df20a 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
preemptively add a new_storage tag
87db523 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@ericl
Copy link
Contributor Author

ericl commented Aug 21, 2023

Tests look good to me.

justinvyu
justinvyu approved these changes Aug 21, 2023
View reviewed changes
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.

Ok to merge after tests pass.

@justinvyu justinvyu mentioned this pull request Aug 21, 2023
Closed
@ericl ericl merged commit 56d6667 into ray-project:master Aug 21, 2023
60 of 65 checks passed
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
@ericl @arvind-chandra
[tune/train] Get rid of downsyncing path and download just the result…
8c7387c 
….json / result.csv on result (ray-project#38598)

Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
@ericl
[tune/train] Get rid of downsyncing path and download just the result…
ae81d54 
….json / result.csv on result (ray-project#38598)

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
Reviewers

@justinvyu justinvyu justinvyu approved these changes

Assignees

@justinvyu justinvyu

@krfricke krfricke

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[tune/train] Get rid of downsyncing path and download just the result.json / result.csv on result
3 participants
@ericl @justinvyu @krfricke