-
-
Notifications
You must be signed in to change notification settings - Fork 644
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
Enable %f pattern in now resolver #1287
Conversation
Thanks @forcecore Could you add a test for checking that %f is correctly resolved? Maybe we could add this to the docs at https://hydra.cc/docs/next/configure_hydra/workdir |
@shagunsodhani Maybe along with |
Maybe something like this https://github.com/facebookresearch/hydra/blob/master/tests/test_hydra.py#L718 ? |
tests/test_hydra.py
Outdated
] | ||
print(" ".join(cmd)) | ||
out, _err = get_run_output(cmd) | ||
print(out) |
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.
remove the prints please.
tests/test_hydra.py
Outdated
rundir = str(tmpdir / rundir) | ||
cmd = [ | ||
"tests/test_apps/run_dir_test/my_app_microseconds.py", | ||
f"hydra.run.dir={rundir}", | ||
] |
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.
You can use integration_test() instead of this decicated app.
Take a look at how it's being used in other places.
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.
It seems to expect something exact, but with microseconds I'm unsure how timestamp will go?
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.
You could print something like:
'%f' not in os.getcwd()
and ensures that it's "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.
Then I might try assert_regex_match, then it seems to only have use cases with dedicated programs
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.
You could print something like:
'%f' not in os.getcwd() and ensures that it's "True".
OK I see
I think the new test works. It fails with the old code and passes with the new one. |
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.
- Please update the doc at https://hydra.cc/docs/configure_hydra/intro
- Please add a news fragment file. See this.
|
Wrong doc link, should be https://docs.python.org/3/library/datetime.html#strftime-and-strptime-behavior |
modified: website/docs/configure_hydra/Intro.md
Doc link updated |
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.
Awesome, thanks!
Motivation
I sometimes manually run multiple instances of Hydra configured programs within a fraction of a second. In that case, their output directory's timestamp collide.
The user manual points to python2 strftime document which supports %f pattern (micro seconds), which would be one way of solving this issue.
https://docs.python.org/2/library/datetime.html#strftime-strptime-behavior
However, in Python 3,
https://docs.python.org/3/library/time.html?highlight=strftime#time.strftime
time.strftime does not implement %f format and datetime.stftime does.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
(How should this PR be tested? Do you require special setup to run the test or repro the fixed bug?)
Simple tutorial codes in the user manual would do the job
main.py:
config.yaml:
Related Issues and PRs
(Is this PR part of a group of changes? Link the other relevant PRs and Issues here. Use https://help.github.com/en/articles/closing-issues-using-keywords for help on GitHub syntax)
None related