search

facebookresearch / mmf

A modular framework for vision & language multimodal research from Facebook AI Research (FAIR)
https://mmf.sh/
Other
5.49k stars 935 forks source link

[refactor] Extend WandbLogger to log config variables, entity and kwargs #1129

Closed ayulockin closed 2 years ago

ayulockin commented 2 years ago

  1. I was going through the WandbLogger class that was recently added to this library. Since MMF is a config-first framework, it would be great to log the config file to keep track of what's going in the model training/evaluation process.

    The effect of this small change can be seen through the before and after screenshots. One can compare one experiment from another by comparing the config.

    Before: image

    After: image

  2. I have also added another argument to pass entity to wandb.init. Explicit mention of this will help users who are using a W&B Teams account.

  3. Extra arguments to the wandb.init() can be passed via the config file.

  4. Ability to log learning rate over time.

PS: I am learning about this library and will be using it for personal work. Since I am also affiliated with Weights and Biases, I will be creating some PRs to add functionalities to the logger that might be useful for everyone. :)

ayulockin commented 2 years ago

Hey @ebsmothers, I cleaned the code a bit more and updated the docs. Would love your feedback.

ayulockin commented 2 years ago

I have made the changes as per your suggestions. I just explicitly defined entity and project arguments in the WandbLogger. All other arguments are parsed using omegaconf the way it's used here but added this in the WandbLogger itself so that in future I can keep extending the same.

I have also updated the docs to reflect the same.

facebook-github-bot commented 2 years ago

@ebsmothers has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ayulockin commented 2 years ago

Hey @ebsmothers is there any update on this PR? Would love to know if I need to make any changes. :)

facebook-github-bot commented 2 years ago

@ayulockin has updated the pull request. You must reimport the pull request before landing.

ebsmothers commented 2 years ago

Hi @ayulockin sorry for the delay and thanks for your patience. After a bit more consideration I decided we do not need to use open_dict for this case. To save you the trouble, I went ahead and updated the PR myself (with one other minor change). Thanks :)

facebook-github-bot commented 2 years ago

@ebsmothers has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented 2 years ago

@ayulockin has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot commented 2 years ago

@ebsmothers has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented 2 years ago

@ayulockin has updated the pull request. You must reimport the pull request before landing.

ayulockin commented 2 years ago

Hey @ebsmothers, the change to deepcopy(config.training.wandb) threw an error: "DictConfig has no attribute pop" when wandb_kwargs.pop("enabled") was called so changed the type to dict.

facebook-github-bot commented 2 years ago

@ebsmothers has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ayulockin commented 2 years ago

Closing this PR in favor of #1137.