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

fix add_config func is not called bug #613

Merged
merged 10 commits into from Oct 24, 2022
Merged

fix add_config func is not called bug #613

merged 10 commits into from Oct 24, 2022

Conversation

shenmishajing
Copy link
Contributor

Motivation

The add_config func of visualizer is not called, so all vis_backends will not get the configs.
call self.visualizer.add_config(self.cfg) after create visualizer.

The add_config func of wandb is wrong, see https://docs.wandb.ai/guides/track/config
wandb add_config by wandb.config.update(<a dict object here>) instead of wandb.save()

In addition, wandb can log all codes to reproduce experiments, I add this feature in add_config func.

Modification

call self.visualizer.add_config(self.cfg) after create visualizer.
fix wandb add_config bug by wandb.config.update(<a dict object here>) instead of wandb.save()
add wandb log_code feature

Use cases (Optional)

set the log_code_name of WandbVisBackend to named code artifacts.

Checklist

  1. The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  2. The documentation has been modified accordingly, like docstring or example tutorials.

fix wandb add_config bug
add wandb log_code feature
@zhouzaida
Copy link
Member

Hi @shenmishajing , thanks for your contribution. It would make the code more robust if unit tests are added for those modifications.

shenmishajing and others added 4 commits October 16, 2022 09:43
add log_code_name param to docstring
Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>
Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>
@shenmishajing
Copy link
Contributor Author

@zhouzaida How about this? I add a registry name PATH_FILTERS to specific the include_fn and exclude_fn.

@zhouzaida
Copy link
Member

Sorry, I may have misrepresented it, what I was trying to say was that you could follow your previous solution and not support include_func and exclude_func.

@shenmishajing
Copy link
Contributor Author

Sorry, I may have misrepresented it, what I was trying to say was that you could follow your previous solution and not support include_func and exclude_func.

@zhouzaida Oh……I'm sorry I ignore the no need's no in your comment. So, should I roll it back?

@zhouzaida
Copy link
Member

Sorry, I may have misrepresented it, what I was trying to say was that you could follow your previous solution and not support include_func and exclude_func.

@zhouzaida Oh……I'm sorry I ignore the no need's no in your comment. So, should I roll it back?

Suggest rolling it back.

@shenmishajing
Copy link
Contributor Author

Sorry, I may have misrepresented it, what I was trying to say was that you could follow your previous solution and not support include_func and exclude_func.

@zhouzaida Oh……I'm sorry I ignore the no need's no in your comment. So, should I roll it back?

Suggest rolling it back.

@zhouzaida Got it.

@ZwwWayne ZwwWayne merged commit d270516 into open-mmlab:main Oct 24, 2022
C1rN09 pushed a commit to C1rN09/mmengine that referenced this pull request Nov 1, 2022
* fix add_config func is not called bug
fix wandb add_config bug
add wandb log_code feature

* move log_code_name param to the last one
add log_code_name param to docstring

* add config only when there is a cfg

Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>

* add unit test for log_code_name param of WandbVisBackend

* Update mmengine/visualization/vis_backend.py

Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>

* use log_code_kwargs instead of only log_code_name
add PATH_FILTERS registry

* use log_code_kwargs instead of only log_code_name
add PATH_FILTERS registry

* fix add config unit test

* roll back to log_code_name version

Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>
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.

None yet

4 participants