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

ayulockin · 2021-10-20T13:47:24Z

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:

After:
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.
Extra arguments to the wandb.init() can be passed via the config file.
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. :)

…rgs (#1) ability to log config file, initialize wandb with kwargs and pass entity argument for teams account.

ebsmothers

Thanks for contributing to MMF. The changes look good overall. One small request: can you also update the docs to improve the visibility of the change?

…rics, log lr

ayulockin · 2021-10-25T20:58:50Z

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

ebsmothers

Thanks for updating the diff. I've left a couple suggestions on how we can better handle init_kwargs, please let me know if anything is unclear :)

ebsmothers · 2021-10-26T04:12:08Z

mmf/utils/logger.py

+        self._wandb_init = dict(
+            entity=entity, name=name, project=project, dir=save_dir, config=config
+        )

+        init_kwargs = dict(
+            itertools.islice(
+                config.training.wandb.items(), 4, len(config.training.wandb)
+            )
+        )
        self._wandb_init.update(**init_kwargs)


Actually I think the previous version was better, since now we are assuming a specific ordering in the WandB training config (i.e. that the keys corresponding to init_kwargs will come last, which in general will not be true).

Suggested change

self._wandb_init = dict(

entity=entity, name=name, project=project, dir=save_dir, config=config

)

init_kwargs = dict(

itertools.islice(

config.training.wandb.items(), 4, len(config.training.wandb)

)

)

self._wandb_init.update(**init_kwargs)

self._wandb_init = dict(**init_kwargs)

This makes sense. Yes removing this assumption is better.

ebsmothers · 2021-10-26T04:21:26Z

mmf/trainers/callbacks/logistics.py

@@ -58,11 +58,12 @@ def __init__(self, config, trainer):
            if env_wandb_logdir:
                log_dir = env_wandb_logdir

-            wandb_projectname = config.training.wandb.wandb_projectname
-            wandb_runname = config.training.wandb.wandb_runname
-
            self.wandb_logger = WandbLogger(


Here you can just pass in wandb init_kwargs straightaway from config.training.wandb (without explicitly declaring entity, project, ... fields). Just unpack config.training.wandb and pass it all in at once, similar to what's done with lightning_params_dict here

Thanks for sharing the code snippet. I will make this change and commit.

ebsmothers · 2021-10-26T04:24:02Z

mmf/utils/logger.py


    Raises:
        ImportError: If wandb package is not installed.
    """

    def __init__(
        self,
+        entity: Optional[str] = None,


Here you can just keep **init_kwargs and remove all the other parameters. This way it will be more flexible, and users can still find all the relevant fields enumerated in the docs.

Will it be okay if we can at least keep the current set of parameters and add **init_kwargs? Explicit mention of some of the arguments will be useful for the users in my opinion, especially, entity since anyone willing to log to a team account needs this and project to pass in the name of the project.

As per your suggestion, we can achieve wandb.init() just by passing **init_kwargs. It's just a matter of explicit vs implicit mention of a few important args.

ayulockin · 2021-10-26T12:35:10Z

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 · 2021-10-26T15:02:35Z

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

ayulockin · 2021-11-01T14:35:33Z

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

facebook-github-bot · 2021-11-01T16:41:08Z

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

ebsmothers · 2021-11-01T16:44:54Z

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 · 2021-11-01T16:45:36Z

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

mmf/utils/logger.py

facebook-github-bot · 2021-11-01T18:37:26Z

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

facebook-github-bot · 2021-11-01T18:43:23Z

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

facebook-github-bot · 2021-11-09T21:04:15Z

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

ayulockin · 2021-11-09T21:07:00Z

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 · 2021-11-10T23:17:14Z

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

Summary: 🚀 I have extended the `WandbLogger` with the ability to log the `current.pt` checkpoint as W&B Artifacts. Note that this PR is based on top of this [PR](#1129). ### What is W&B Artifacts? > W&B Artifacts was designed to make it effortless to version your datasets and models, regardless of whether you want to store your files with us or whether you already have a bucket you want us to track. Once you've tracked your dataset or model files, W&B will automatically log each and every modification, giving you a complete and auditable history of changes to your files. Through this PR, W&B Artifacts can help save and organize machine learning models throughout a project's lifecycle. More details in the documentation [here](https://docs.wandb.ai/guides/artifacts/model-versioning). ### Modification This PR adds a `log_model_checkpoint` method to the `WandbLogger` class in the `utils/logger.py` file. This method is called in the `utils/checkpoint.py` file. ### Usage To use this, in the `config/defaults.yaml` do, `training.wandb.enabled=true` and `training.wandb.log_checkpoint=true`. ### Result The screenshot shows the `current.pt` checkpoints saved at intervals defined by `training.checkpoint_interval`. You can check out the logged artifacts page [here](https://wandb.ai/ayut/mmf/artifacts/model/run_ey9xextf_model/0dc64164acbdc300fd01/api). ![image](https://user-images.githubusercontent.com/31141479/139390462-d5c8445e-5c20-4fdd-85d0-51ef64846bf0.png) ### Superpowers With this small addition, now one can easily track different versions of the model, download a checkpoint of interest by using the API in the API tab, easily share the checkpoints with teammates, etc. ### Requests This is a draft PR as there are a few more things that can be improved here. * Is there a better way to access the path to the `current.pt` checkpoint? Rather is the modification made to `utils/checkpoint.py` an acceptable way of approaching this? * While logging a file as W&B artifacts we can also provide metadata associated with that file. In this case, we can add current iteration, training metrics, etc. as the metadata. Would love to get suggestions about the different data points that I should log as metadata alongside the checkpoints. * How to determine if a checkpoint is the best one? If a checkpoint is best I can add `best` as an alias for that checkpoint's artifact. Pull Request resolved: #1137 Test Plan: Imported from GitHub, without a `Test Plan:` line. **Static Docs Preview: mmf** |[Full Site](https://our.intern.facebook.com/intern/staticdocs/eph/D32402090/V6/mmf/)| |**Modified Pages**| |[docs/notes/logger](https://our.intern.facebook.com/intern/staticdocs/eph/D32402090/V6/mmf/docs/notes/logger/)| Reviewed By: apsdehal Differential Revision: D32402090 Pulled By: ebsmothers fbshipit-source-id: 94b881ec55c4197301331d571bc926521e2feecc

ayulockin · 2021-11-24T15:53:38Z

Closing this PR in favor of #1137.

[refactor] Extend WandbLogger to log config variables, entity and kwa…

78b1428

…rgs (#1) ability to log config file, initialize wandb with kwargs and pass entity argument for teams account.

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 20, 2021

Merge branch 'facebookresearch:main' into main

f879c53

ebsmothers reviewed Oct 22, 2021

View reviewed changes

ayulockin and others added 3 commits October 25, 2021 17:57

Merge branch 'facebookresearch:main' into main

1a74a25

cleaned passing of kwargs, added wandb_logger to write validation met…

a0decd2

…rics, log lr

update docs

a97cf2a

ebsmothers requested changes Oct 26, 2021

View reviewed changes

init kwargs (#3)

5cc98b8

ayulockin mentioned this pull request Oct 29, 2021

[feat] Model version control using W&B Artifacts #1137

Closed

replace usage of open_dict

5dbbbd8

ayulockin commented Nov 1, 2021

View reviewed changes

mmf/utils/logger.py Outdated Show resolved Hide resolved

Add config back to WandB init

23addf0

minor change to correct the error it was throwing (#4)

2571772

ayulockin closed this Nov 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ayulockin commented Oct 20, 2021 •

edited

Loading

ebsmothers left a comment

ayulockin commented Oct 25, 2021

ebsmothers left a comment

ebsmothers Oct 26, 2021

ayulockin Oct 26, 2021

ebsmothers Oct 26, 2021

ayulockin Oct 26, 2021

ebsmothers Oct 26, 2021

ayulockin Oct 26, 2021

ayulockin commented Oct 26, 2021 •

edited

Loading

facebook-github-bot commented Oct 26, 2021

ayulockin commented Nov 1, 2021 •

edited

Loading

facebook-github-bot commented Nov 1, 2021

ebsmothers commented Nov 1, 2021

facebook-github-bot commented Nov 1, 2021

facebook-github-bot commented Nov 1, 2021

facebook-github-bot commented Nov 1, 2021

facebook-github-bot commented Nov 9, 2021

ayulockin commented Nov 9, 2021

facebook-github-bot commented Nov 10, 2021

ayulockin commented Nov 24, 2021

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

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

Conversation

ayulockin commented Oct 20, 2021 • edited Loading

ebsmothers left a comment

Choose a reason for hiding this comment

ayulockin commented Oct 25, 2021

ebsmothers left a comment

Choose a reason for hiding this comment

ebsmothers Oct 26, 2021

Choose a reason for hiding this comment

ayulockin Oct 26, 2021

Choose a reason for hiding this comment

ebsmothers Oct 26, 2021

Choose a reason for hiding this comment

ayulockin Oct 26, 2021

Choose a reason for hiding this comment

ebsmothers Oct 26, 2021

Choose a reason for hiding this comment

ayulockin Oct 26, 2021

Choose a reason for hiding this comment

ayulockin commented Oct 26, 2021 • edited Loading

facebook-github-bot commented Oct 26, 2021

ayulockin commented Nov 1, 2021 • edited Loading

facebook-github-bot commented Nov 1, 2021

ebsmothers commented Nov 1, 2021

facebook-github-bot commented Nov 1, 2021

facebook-github-bot commented Nov 1, 2021

facebook-github-bot commented Nov 1, 2021

facebook-github-bot commented Nov 9, 2021

ayulockin commented Nov 9, 2021

facebook-github-bot commented Nov 10, 2021

ayulockin commented Nov 24, 2021

ayulockin commented Oct 20, 2021 •

edited

Loading

ayulockin commented Oct 26, 2021 •

edited

Loading

ayulockin commented Nov 1, 2021 •

edited

Loading