fixes to allow tune/tests/test_commands.py to run on windows #21342
Conversation
mattip
force-pushed
the
test_syncer
branch
2 times, most recently
from
5fcfd5a
to
b689ae9
Compare
python/ray/tune/syncer.py
Outdated
| @@ -440,6 +440,8 @@ def get_node_syncer(local_dir, remote_dir=None, sync_function=None): | |||
| return _syncers[key] | |||
| elif not remote_dir or sync_function is False: | |||
| sync_client = NOOP | |||
| elif local_dir == remote_dir: | |||
There was a problem hiding this comment.
@richardliaw Does this change look good to you? This is to make the tune CLI work locally on windows (if there is no rsync available, e.g. on windows). Is that valueable or should we try to get a platform independent way to sync the results?
There was a problem hiding this comment.
Unfortunately, I think this would essentially disable all our p2p syncing (for all platforms). local_dir and remote_dir don't include ip addresses for nodes. For most cases, they are equal. Can we just rely on setting TUNE_DISABLE_AUTO_CALLBACK_SYNCER to avoid windows rsync issue?
Why all unit tests pass is eluding me. @krfricke thoughts?
There was a problem hiding this comment.
OK, I will revert this and will try to enable TUNE_DISABLE_AUTO_CALLBACK_SYNCER to avoid windows calling into rsync. Thanks for the review.
There was a problem hiding this comment.
Btw the reason why this probably still works for unit testing is that we don't have proper sync testing in place here. Release tests would probably fail with this test.
With our new local multinode testing we should be able to test these things in the near future.
|
All checks have passed. |
|
I removed the change to syncer which apparently was not needed to close #20747, and rebased off latest master. |
Why are these changes needed?
tunedoes not run smoothly on Windows. This cleans up some blockers:shutils.get_terminal_sizeinstead ofPopen(stty)Trainer.workersis None at the end oftest_commands.py, so the cleanup command was erroring. The error was not fatal, but was printing in the logs.rync-based syncing solution is not needed. This is the real fix for issue [Bug] ERROR syncer.py:75 -- Log sync requires rsync to be installed. but newest ray doesn't need it #20747Related issue number
Closes #20747
Checks
scripts/format.shto lint the changes in this PR.Should I add python/ray/tune/tests/test_commands.py which passes to the windows CI?