Add support for Weights and Biases #3417
Conversation
|
Thanks for making the PR @stephenroller! Really excited about this. |
|
Wow, glad to see this going in! Thank you, @stephenroller! |
stephenroller
requested review from
emilydinan and
jxmsML
and removed request for
klshuster
parlai/core/logs.py
Outdated
| """ | ||
| Add w&b CLI args. | ||
| """ | ||
| logger = parser.add_argument_group('Tensorboard Arguments') |
There was a problem hiding this comment.
nit: still says Tensorboard
parlai/core/logs.py
Outdated
| '--wandb-log', | ||
| type='bool', | ||
| default=False, | ||
| help="Enable W&B logging of metrics, default is %(default)s", |
There was a problem hiding this comment.
why is default false here? LOL maybe same question, why is tensorboard_log default False
There was a problem hiding this comment.
i'm good with keeping it False, since it requires additional overhead/setup that is not necessary to just train a model
parlai/core/logs.py
Outdated
|
|
||
| def __init__(self, opt: Opt, model=None): | ||
| try: | ||
| # tensorboard is a very expensive thing to import. Wait until the |
There was a problem hiding this comment.
nit: tensorboard in the comment
| raise ImportError('Please run `pip install wandb`.') | ||
|
|
||
| name = opt.get('wandb_name') | ||
| project = opt.get('wandb_project') or datetime.datetime.now().strftime( |
There was a problem hiding this comment.
- if it is set by timestamps, what if my job gets pre-empted, would it setup a new
wandbloggerthen in this case? - Just curious if we need to set
reinit=Falsehere in case of preempted jobs.
There was a problem hiding this comment.
- In our sweep setup, I have this parameter automatically set to the sweep name. So we should be good
- Good question. Let's try it and see what happens.
parlai/core/logs.py
Outdated
| type='bool', | ||
| default=False, | ||
| help="Enable W&B logging of metrics, default is %(default)s", | ||
| hidden=False, |
There was a problem hiding this comment.
is there a reason we're specifying hidden=False? just for verbosity?
parlai/core/logs.py
Outdated
| '--wandb-log', | ||
| type='bool', | ||
| default=False, | ||
| help="Enable W&B logging of metrics, default is %(default)s", |
There was a problem hiding this comment.
i'm good with keeping it False, since it requires additional overhead/setup that is not necessary to just train a model
parlai/core/logs.py
Outdated
|
|
||
| def log_metrics(self, setting, step, report): | ||
| """ | ||
| Add all metrics from tensorboard_metrics opt key. |
|
Requesting a re-review for sanity. I think I'd land this fast and start crowdsourcing bugs instead of manually testing myself. |
parlai/core/logs.py
Outdated
| """ | ||
| Add w&b CLI args. | ||
| """ | ||
| logger = parser.add_argument_group('Tensorboard Arguments') |
There was a problem hiding this comment.
nit: still says Tensorboard
| '--wandb-name', | ||
| type=str, | ||
| default=None, | ||
| help='W&B run name. If not set, WandB will randomly generate a name.', |
There was a problem hiding this comment.
what does a randomly generated name look like?
There was a problem hiding this comment.
like "peachy-armadillo-1" or sth like that
For our internal sweeps, it's set to the name of the directory (e.g. hashname)
| for key, value in opt.items(): | ||
| if value is None or isinstance(value, (str, numbers.Number, tuple)): | ||
| setattr(self.run.config, key, value) | ||
| if model is not None: |
There was a problem hiding this comment.
non torchagent models I believe
|
Wtf |
what? |
Patch description
Thanks to @annirudh for making the initial version of this a long time ago, and to @lavanyashukla for support internally. We've been playing with it internally and find W&B to be an excellent addition now that our metrics have grown.
Testing steps
Manual testing.