-
Notifications
You must be signed in to change notification settings - Fork 6k
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.
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
[tune] Allow relative local_dir at tune.run() #4734
[tune] Allow relative local_dir at tune.run() #4734
Conversation
Test PASSed. |
Test FAILed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test PASSed. |
Test FAILed. |
@richardliaw Remember the PR #4733 that I formatted the files using In this PR the travis failed due to the same issue:
I check the travis log and it turn out to format the codes in the same way as yours, that is:
However, in my macOS Mojave with Python 3.6.0 and latest Ray environment, I still get the following formatted codes after running format script. Is that because some mistakes I made when set up the Ray?
|
Test PASSed. |
Test FAILed. |
@pengzhenghao one problem with the linting could be wrong yapf version. What's the output of your |
|
Test PASSed. |
Test FAILed. |
shutil.rmtree(absolute_local_dir, ignore_errors=True) | ||
|
||
def testAbsolutePath(self): | ||
local_dir = "~/test_absolute_local_dir" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local_dir = "~/test_absolute_local_dir" | |
local_dir = "~/test_absolute_local_dir" | |
self.assertFalse(os.path.exists(local_dir)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A failed run of test_tune_save_restore.py
may left this dir ~/test_absolute_local_dir
existing. Therefore I remove the potentially existing dir before training, otherwise you have to delete it by hand.
def _train(self, exp_name, local_dir, absolute_local_dir):
shutil.rmtree(absolute_local_dir, ignore_errors=True)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to handle the failure cleanly to remove the directory, and then check the non-existence of it. This just because, I think deleting directories from users' home directory, even with hard-coded names like this, is kinda precarious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can add self.assertFalse(os.path.exists("~/test_absolute_local_dir"))
in setUp
and shutil.rmtree("~/test_absolute_local_dir", ignore_errors=True)
in tearDown
or something similar?
Test PASSed. |
Test FAILed. |
Test PASSed. |
I still don't know why the builds using python 3.5 always stuck. Why it so special? |
Test FAILed. |
Test PASSed. |
Test FAILed. |
Test FAILed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
What do these changes do?
Allow the relative local_dir for
tune.run()
or Experiment.Add unit test for both relative
local_dir
and absolutelocal_dir
.@hartikainen You said using the variables' names helps for debugging. However I think the restoring log information may be more helpful using the human-understandable names, for those users who might need to know whether they are using the correct checkpoint.
Related issue number
The same as #4725
Closes #4724
Linter
scripts/format.sh
to lint the changes in this PR.