Skip to content
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

Add feature : config file dump to wandb server #1471

Closed
wants to merge 12 commits into from

Conversation

hyun06000
Copy link

@hyun06000 hyun06000 commented Nov 9, 2021

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

WandB has many features to record ML experiments. It can record python scripts or any files on WandB repository as well as plots of training and measurements. So I want to add a feature to upload a config file of mmdetection or mmsegmentation etc. The config file is the most important part of open-mmlab. This pull-request can support a model comparison and reconstruction at the same time at wandb.

Modification

file: mmcv/runner/hooks/logger/wandb.py

line: 2 / 20 / 27 / 47 / 48

Use cases (Optional)

image
This is an example of config file ofr wandb logger. If user can set a work-dir before setting config file, user can write down the dumped config file path directly.

image
every mm{library}/tools/train.py includes a line for dump the config file of each experiment. So user can set a line to write a config path for wandb under the line.

config_dump_path = osp.join(cfg.work_dir, osp.basename(args.config))
cfg.dump(config_dump_path)
cfg.log_config.hooks[2].config_path = config_dump_path

This is a result.
image

image

Checklist

Before PR:

  • I have read and followed the workflow indicated in the CONTRIBUTING.md to create this PR.
  • Pre-commit or linting tools indicated in CONTRIBUTING.md are used to fix the potential lint issues.
  • Bug fixes are covered by unit tests, the case that causes the bug should be added in the unit tests.
  • New functionalities are covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  • The documentation has been modified accordingly, including docstring or example tutorials.

After PR:

  • If the modification has potential influence on downstream or other related projects, this PR should be tested with some of those projects, like MMDet or MMCls.
  • CLA has been signed and all committers have signed the CLA in this PR.

@CLAassistant
Copy link

CLAassistant commented Nov 9, 2021

CLA assistant check
All committers have signed the CLA.

@zhouzaida
Copy link
Collaborator

cfg.log_config.hooks[2].config_path = config_dump_path

The above line may not be the best way to set the config_path. Maybe we can manually set the config_path in config files or get the value from the runner but we need to add it to runner in the downstream codebases. I prefer the former.

@zhouzaida
Copy link
Collaborator

cfg.log_config.hooks[2].config_path = config_dump_path

The above line may not be the best way to set the config_path. Maybe we can manually set the config_path in config files or get the value from the runner but we need to add it to runner in the downstream codebases. I prefer the former.

But the former way is not a convenient way to set the config_path which requires us to manually set it every time when we launch another experiment.

@hyun06000
Copy link
Author

hyun06000 commented Nov 9, 2021

cfg.log_config.hooks[2].config_path = config_dump_path

The above line may not be the best way to set the config_path. Maybe we can manually set the config_path in config files or get the value from the runner but we need to add it to runner in the downstream codebases. I prefer the former.

But the former way is not a convenient way to set the config_path which requires us to manually set it every time when we launch another experiment.

I agree that to do something to runner for this feature is not efficient way and the former is better method.

Actually I set the config for each experiment when there was no hyperparameter auto tunning. But when I used a hyperparameter tunning tool (wandb sweep), the tool was worked with a train script which includes adjusting and dumping config. In my opinion, this feature is more useful with hyperparameter tunning. That is why I write the line cfg.log_config.hooks[2].config_path = config_dump_path.

@zhouzaida
Copy link
Collaborator

Actually I set the config for each experiment when there was no hyperparameter auto tunning. But when I used a hyperparameter tunning tool (wandb sweep), the tool was worked with a train script which includes adjusting and dumping config. In my opinion, this feature is more useful with hyperparameter tunning. That is why I write the line cfg.log_config.hooks[2].config_path = config_dump_path.

Got it. We need to add a unit test for the feature.

mmcv/runner/hooks/logger/wandb.py Outdated Show resolved Hide resolved
mmcv/runner/hooks/logger/wandb.py Outdated Show resolved Hide resolved
tests/test_runner/test_hooks.py Outdated Show resolved Hide resolved
@hyun06000
Copy link
Author

@zhouzaida I believed the malfunction was from my mistakes, however, as the results of some tests, I found this function does not work with a path above wandb.run.dir. The wandb.save needs glob_str and base_path to find a file to upload. I found that the glob_str is not real global path for user's file system. The wandb.run.dir is root for wandb.run.
image

This image is the result of wandb.save with a glob_str above the root for wandb.run(I mean wandb.run.dir). When I set the path of a tartget file with os.path.dirname(), the function doesn't save any file. Also, without base_path, the function showed the error like below.
image

Therefore, we have two choice whether to move wandb.run.dir or to make copy in wandb.run.dir. In my opinion, the latter is better than the former in general cases.

@hyun06000 hyun06000 requested a review from zhouzaida November 30, 2021 00:57
@fcakyon
Copy link
Contributor

fcakyon commented Dec 2, 2021

@hyun06000 @zhouzaida this feature is so much needed, cant wait to see this merged!

@zhouzaida
Copy link
Collaborator

zhouzaida commented Dec 2, 2021

Sorry for the late reply

@zhouzaida
Copy link
Collaborator

@hyun06000 @zhouzaida this feature is so much needed, cant wait to see this merged!

Got it.

@zhouzaida
Copy link
Collaborator

@zhouzaida I believed the malfunction was from my mistakes, however, as the results of some tests, I found this function does not work with a path above wandb.run.dir. The wandb.save needs glob_str and base_path to find a file to upload. I found that the glob_str is not real global path for user's file system. The wandb.run.dir is root for wandb.run. image

This image is the result of wandb.save with a glob_str above the root for wandb.run(I mean wandb.run.dir). When I set the path of a tartget file with os.path.dirname(), the function doesn't save any file. Also, without base_path, the function showed the error like below. image

Therefore, we have two choice whether to move wandb.run.dir or to make copy in wandb.run.dir. In my opinion, the latter is better than the former in general cases.

Yet. As you said, If we do not provide the base_path parameter, any folder will not be created which means all files will be uploaded to the same level directory root. It is not an expected behavior. So we can do it ourselves. If the config_path is a directory, we can set the base_path as config_path and the glob_str as *. If the config_path is a file, we can set the base_path as dirname(config_path) and glob_str as basename(config_path).

More details about wandb.save can be found at https://github.com/wandb/client/blob/v0.12.7/wandb/sdk/wandb_run.py#L1349-L1438.

@hyun06000
Copy link
Author

@zhouzaida I believed the malfunction was from my mistakes, however, as the results of some tests, I found this function does not work with a path above wandb.run.dir. The wandb.save needs glob_str and base_path to find a file to upload. I found that the glob_str is not real global path for user's file system. The wandb.run.dir is root for wandb.run. image
This image is the result of wandb.save with a glob_str above the root for wandb.run(I mean wandb.run.dir). When I set the path of a tartget file with os.path.dirname(), the function doesn't save any file. Also, without base_path, the function showed the error like below. image
Therefore, we have two choice whether to move wandb.run.dir or to make copy in wandb.run.dir. In my opinion, the latter is better than the former in general cases.

Yet. As you said, If we do not provide the base_path parameter, any folder will not be created which means all files will be uploaded to the same level directory root. It is not an expected behavior. So we can do it ourselves. If the config_path is a directory, we can set the base_path as config_path and the glob_str as *. If the config_path is a file, we can set the base_path as dirname(config_path) and glob_str as basename(config_path).

More details about wandb.save can be found at https://github.com/wandb/client/blob/v0.12.7/wandb/sdk/wandb_run.py#L1349-L1438.

First of all, I'm sorry for the late response.
The link was very useful for me to understand wandb.save. Thanks.

@zhouzaida
Copy link
Collaborator

Hi, the CI failed.

@zhouzaida
Copy link
Collaborator

PR #1616 has supported uploading files to the wandb server so should I close the PR.

@zhouzaida zhouzaida closed this Jan 15, 2022
@OpenMMLab-Assistant005
Copy link

Hi @hyun06000 !We are grateful for your efforts in helping improve this open-source project during your personal time.
Welcome to join OpenMMLab Special Interest Group (SIG) private channel on Discord, where you can share your experiences, ideas, and build connections with like-minded peers. To join the SIG channel, simply message moderator— OpenMMLab on Discord or briefly share your open-source contributions in the #introductions channel and we will assist you. Look forward to seeing you there! Join us :https://discord.gg/UjgXkPWNqA
If you have a WeChat account,welcome to join our community on WeChat. You can add our assistant :openmmlabwx. Please add "mmsig + Github ID" as a remark when adding friends:)
Thank you again for your contribution❤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants