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 graph func is not called bug #632

Merged
merged 8 commits into from
Nov 21, 2022
Merged

fix add graph func is not called bug #632

merged 8 commits into from
Nov 21, 2022

Conversation

shenmishajing
Copy link
Contributor

Motivation

Fix add graph func is not called bug. See issue #621

Modification

  • Call visualizer.add_graph after model build.
  • Add watch_kwargs param to WandbVisBackend.

Checklist

  1. The documentation has been modified accordingly, like docstring or example tutorials.

@HAOCHENYE
Copy link
Collaborator

Will it be more proper to call add_graph in NativeVisualizerHook.before_train, and this feature can be enabled by setting
watch_model=True (default False, not sure will it harm the performance)

@HAOCHENYE
Copy link
Collaborator

HAOCHENYE commented Oct 19, 2022

BTW, since add_graph has not been used by the downstream repo, could we rename it to a more proper name? @zhouzaida

@shenmishajing
Copy link
Contributor Author

Will it be more proper to call add_graph in NativeVisualizerHook.before_train, and this feature can be enabled by setting watch_model=True (default False, not sure will it harm the performance)

I agree with that. But, does the NativeVisualizerHook have been added to runner now? I do not find it now, and that's why I add it in runner init directly.

@HAOCHENYE
Copy link
Collaborator

NativeVisualizerHook should be a custom hook. If users need to visualize something, they can inherit or copy NativeVisualizerHook to implement custom VisualizerHook. The code snippet of config to enable VisualizerHook is like this:

config_for_xxx.py

custom_hooks = [
    dict(type='NativeVisualizerHook')
    ...
]

@shenmishajing
Copy link
Contributor Author

shenmishajing commented Oct 19, 2022

NativeVisualizerHook should be a custom hook. If users need to visualize something, they can inherit or copy NativeVisualizerHook to implement custom VisualizerHook. The code snippet of config to enable VisualizerHook is like this:

config_for_xxx.py

custom_hooks = [
    dict(type='NativeVisualizerHook')
    ...
]

Ok, got it. I think it's more proper to call add_graph in NativeVisualizerHook.before_train. By the way, should we move the add_config call to NativeVisualizerHook.before_train as well? @HAOCHENYE

@HAOCHENYE
Copy link
Collaborator

I think add_config is a more commonly used function for the general experiments, it is ok to be called in runner.__init__() .

@shenmishajing
Copy link
Contributor Author

Ok, thanks for your reply.

@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Base: 77.98% // Head: 77.98% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (0a05e71) compared to base (aaba1d8).
Patch coverage: 66.66% of modified lines in pull request are covered.

❗ Current head 0a05e71 differs from pull request most recent head e2acbb2. Consider uploading reports for the commit e2acbb2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #632      +/-   ##
==========================================
- Coverage   77.98%   77.98%   -0.01%     
==========================================
  Files         126      126              
  Lines        9059     9065       +6     
  Branches     1804     1804              
==========================================
+ Hits         7065     7069       +4     
- Misses       1681     1683       +2     
  Partials      313      313              
Flag Coverage Δ
unittests 77.98% <66.66%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmengine/hooks/naive_visualization_hook.py 93.93% <50.00%> (-2.84%) ⬇️
mmengine/visualization/vis_backend.py 84.40% <75.00%> (-0.21%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>
@HAOCHENYE HAOCHENYE added this to the 0.3.0 milestone Oct 27, 2022
@HAOCHENYE HAOCHENYE added the enhancement New feature or request label Oct 27, 2022
hhaAndroid
hhaAndroid previously approved these changes Nov 1, 2022
@zhouzaida zhouzaida linked an issue Nov 1, 2022 that may be closed by this pull request
@zhouzaida
Copy link
Collaborator

Hi @shenmishajing , thanks for your contribution. After having a discussion with WandB team, they have no plan to support visualizing models in wanbd.watch but we will merge this PR because it is a useful feature.

@ZwwWayne ZwwWayne merged commit b7aa4dd into open-mmlab:main Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How does the add_graph func work?
6 participants