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

[train] New persistence mode: Move SyncConfig to train and deprecate Syncer #38855

Merged
merged 30 commits into from
Aug 25, 2023
Merged

[train] New persistence mode: Move SyncConfig to train and deprecate Syncer #38855

merged 30 commits into from
Aug 25, 2023

Conversation

justinvyu
Copy link
Contributor

@justinvyu justinvyu commented Aug 24, 2023 •
edited
Loading

Why are these changes needed?

This PR:

  1. Moves tune.SyncConfig -> train.SyncConfig and soft deprecates tune.SyncConfig.
  2. Soft deprecates useless configs in SyncConfig (sync_on_checkpoint, sync_artifacts, syncer, upload_dir). We should hard-deprecate upload_dir as a follow-up (for 2.7) -- otherwise we need to keep the complicated config resolution logic.
  3. Hard deprecates tune.syncer.Syncer. This is now only used for driver uploading internally (_FilesystemSyncer).
  4. Removes tune.syncer.SyncerCallback (head node syncing mechanism) and all usage of it.
  5. Fixes circular dependencies (train shouldn't import from tune).

Related issue number

Closes #38577
Closes #38865

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

@justinvyu
mv tune.syncer -> train._internal.syncer
5111c23 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
remove train dependencies on tune
36bfe7b 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
remove reenable head node sync flag + some leftover cleanup
ce9150a 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
remove syncer callback pt 1
6b792ca 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
Remove outdated files
f10108e 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
Add train.SyncConfig alias
a51212a 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
remove head node sync deprecation warning deps
5e88387 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
fix imports to use train.SyncConfig
1c94795 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
fix tests depending on syncercallback
5979546 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>

add new storage tag for test_cluster

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
remove test_syncer_callback
55bb02c 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
deprecate args of SyncConfig
bc0db8e 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
remove validate_storage_path usage
dfab96a 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
more targeted warnings
d8f9d2a 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
temp fix for old codepath
d152150 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
softer deprecation for tune.SyncConfig
9605205 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
tune.SyncConfig -> train.SyncConfig
c3f1165 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
justinvyu
justinvyu commented Aug 24, 2023
View reviewed changes
python/ray/train/_internal/syncer.py Outdated
Comment on lines 89 to 99
# TODO(justinvyu): Point users to some user guide for custom fs.
(
"syncer",
"\nPlease implement custom syncing logic with a custom "
"`pyarrow.fs.FileSystem` instead, and pass it into "
"`train.RunConfig(storage_filesystem)`.",
),
("sync_artifacts", ""),
("sync_on_checkpoint", ""),
]:
self._deprecation_warning(attr_name, extra_msg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

soft deprecation warning for these unused params

@justinvyu
Merge branch 'master' of https://github.com/ray-project/ray into air/…
7ace577 
…persistence/deprecate_syncer
@justinvyu
fix test_syncer
eb605df 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
fix test_trainable
bcaa15c 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
fix test_trainer_restore
7f7e5a1 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
fix lint
fb698da 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
address sync config comments
348b448 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu justinvyu requested a review from ericl August 25, 2023 07:57
@justinvyu justinvyu removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 25, 2023
@justinvyu
fix doc
9bfc4fc 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
Merge branch 'master' of https://github.com/ray-project/ray into air/…
70e0431 
…persistence/deprecate_syncer
@justinvyu
Use sync_artifacts_on_checkpoint
4ce65a4 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 25, 2023
@justinvyu
fix validate_save_restore (failed due to artifacts pr)
d4be2b9 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
fix lint
7e3d515 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
krfricke
krfricke approved these changes Aug 25, 2023
View reviewed changes
Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

Changes LGTM, two questions

python/ray/tune/tests/test_multinode_sync.py Show resolved Hide resolved
@justinvyu
Revert "Remove outdated files"
e38b056 
This reverts commit f10108e.

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
Merge branch 'master' of https://github.com/ray-project/ray into air/…
0cef782 
…persistence/deprecate_syncer

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu
enable sync artifacts for test
00c8095 
Signed-off-by: Justin Yu <justinvyu@anyscale.com>
@justinvyu justinvyu removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 25, 2023
@justinvyu justinvyu added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Aug 25, 2023
zhe-thoughts
zhe-thoughts approved these changes Aug 25, 2023
View reviewed changes
Copy link
Collaborator

@zhe-thoughts zhe-thoughts left a comment

Choose a reason for hiding this comment

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

Necessary change for Train in 2.7

@zhe-thoughts zhe-thoughts merged commit 420ce95 into ray-project:master Aug 25, 2023
66 of 72 checks passed
@justinvyu justinvyu deleted the air/persistence/deprecate_syncer branch August 25, 2023 23:46
arvind-chandra pushed a commit to lmco/ray that referenced this pull request Aug 31, 2023
@justinvyu @arvind-chandra
[train] New persistence mode: Move SyncConfig to train and deprecat…
1cb00e5 
…e `Syncer` (ray-project#38855)

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
LeonLuttenberger pushed a commit to jaidisido/ray that referenced this pull request Sep 5, 2023
@justinvyu @LeonLuttenberger
[train] New persistence mode: Move SyncConfig to train and deprecat…
8ca005a 
…e `Syncer` (ray-project#38855)

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
jimthompson5802 pushed a commit to jimthompson5802/ray that referenced this pull request Sep 12, 2023
@justinvyu @jimthompson5802
[train] New persistence mode: Move SyncConfig to train and deprecat…
27c648f 
…e `Syncer` (ray-project#38855)

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
Signed-off-by: Jim Thompson <jimthompson5802@gmail.com>
vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023
@justinvyu
[train] New persistence mode: Move SyncConfig to train and deprecat…
d770bb7 
…e `Syncer` (ray-project#38855)

Signed-off-by: Justin Yu <justinvyu@anyscale.com>
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

@ericl ericl ericl approved these changes

@zhe-thoughts zhe-thoughts zhe-thoughts approved these changes

@krfricke krfricke krfricke approved these changes

@richardliaw richardliaw Awaiting requested review from richardliaw

@gjoliver gjoliver Awaiting requested review from gjoliver

@xwjiang2010 xwjiang2010 Awaiting requested review from xwjiang2010

@amogkam amogkam Awaiting requested review from amogkam

@matthewdeng matthewdeng Awaiting requested review from matthewdeng matthewdeng is a code owner

@Yard1 Yard1 Awaiting requested review from Yard1

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

Assignees

@ericl ericl

@matthewdeng matthewdeng

@zhe-thoughts zhe-thoughts

@krfricke krfricke

Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[train][GA] Add back trial driver syncing behavior [train][GA] Update SyncConfig
5 participants
@justinvyu @ericl @zhe-thoughts @krfricke @matthewdeng