-
Notifications
You must be signed in to change notification settings - Fork 3
[4/9] rm run_name, add_seed_and_date_to_exp_name #18
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
Conversation
d1a5006 to
e231062
Compare
9cfd9a6 to
f205d24
Compare
e231062 to
a2546c2
Compare
f205d24 to
780f13a
Compare
a2546c2 to
bdc2c43
Compare
780f13a to
8bd01d8
Compare
bdc2c43 to
307e01e
Compare
8bd01d8 to
aa6e4b7
Compare
307e01e to
0548cfd
Compare
aa6e4b7 to
380cd14
Compare
0548cfd to
b6d7b83
Compare
380cd14 to
f5460c2
Compare
b6d7b83 to
7a0671a
Compare
d9a59b6 to
5b827c0
Compare
prev-branch: padding-free-squashing-3
5b827c0 to
16a9336
Compare
fabianlim
left a comment
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.
Im not sure if its a good idea to remove run_name
|
Ok, I can undo it. Removing |
|
It's true that removing |
|
So, remove? |
|
oh i didnt know it was a no op.. ok fine lets remove |
fabianlim
left a comment
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.
LGTM
Two related change:
run_namearg inFlatArguments, because the user-supplied value is currently totally ignored and instead overwritten with data from the user-suppliedexp_namearg.--add_seed_and_date_to_exp_nameflag.In upstream, the seed and runtime date are added to the wandb name and (unless
--do_not_randomize_output_dir Trueis specified) the save path. This is hardcoded behavior.This PR allows for skipping this addition by
--add_seed_and_date_to_run_name FalseThis ability is important in (at least) the following two cases: