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

[WIP]Use get_name in forward hook #393

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

NihalHarish
Copy link
Contributor

Description of bug:

  • Users can wrap their modules with helper classes like DataParallelCriterion or DataParallel
custom_loss_module = CustomLossModule()
parallel_custom_loss_module = DataParallelCriterion(custom_loss_module)
  • The smdebug hook register each module like this:
        for name, submodule in module.named_modules():
            assert submodule not in self.module_set, f"Don't register module={module} twice"
            submodule._module_name = name
            self.module_set.add(submodule)
        module._module_name = module._get_name()
        self.module_set.add(module)

The problem with the above line is that the _module_name attribute is attached to the parallel_custom_loss_module and not the nested custom_loss_module.

  • We would have instead needed to do:
submodule.module._module_name = name
  • When the call to the forward hook is made, the helper module will internally call:
    def forward(self, *inputs, **kwargs):
        if not self.device_ids:
            return self.module(*inputs, **kwargs)

We should instead simply use:
module._get_name() in the forward_hook

Style and formatting:

I have run pre-commit install to ensure that auto-formatting happens with every commit.

Issue number, if available

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@NihalHarish NihalHarish changed the title Use get_name in forward hook Use get_name in forward hook Oct 30, 2020
@NihalHarish NihalHarish changed the title Use get_name in forward hook Use get_name in forward hook Oct 30, 2020
@codecov-io
Copy link

codecov-io commented Oct 30, 2020

Codecov Report

Merging #393 into master will decrease coverage by 2.85%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
- Coverage   85.49%   82.63%   -2.86%     
==========================================
  Files          86       86              
  Lines        6514     6520       +6     
==========================================
- Hits         5569     5388     -181     
- Misses        945     1132     +187     
Impacted Files Coverage Δ
smdebug/pytorch/hook.py 0.00% <0.00%> (-82.41%) ⬇️
smdebug/pytorch/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
smdebug/pytorch/singleton_utils.py 0.00% <0.00%> (-100.00%) ⬇️
smdebug/pytorch/collection.py 0.00% <0.00%> (-90.00%) ⬇️
smdebug/rules/action/stop_training_action.py 56.45% <0.00%> (-20.97%) ⬇️
smdebug/pytorch/utils.py 0.00% <0.00%> (-18.52%) ⬇️
smdebug/rules/req_tensors.py 79.16% <0.00%> (-11.12%) ⬇️
smdebug/core/tfevent/util.py 92.00% <0.00%> (-8.00%) ⬇️
smdebug/tensorflow/callable_cache.py 78.26% <0.00%> (-4.35%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be862bd...3c9b1ad. Read the comment docs.

@NihalHarish NihalHarish changed the title Use get_name in forward hook [WIP]Use get_name in forward hook Oct 30, 2020
Copy link
Contributor

@Vikas-kum Vikas-kum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if module.modules will work, as it will give you modules recursively. https://discuss.pytorch.org/t/module-children-vs-module-modules/4551

@@ -215,9 +224,9 @@ def register_module(self, module):

for name, submodule in module.named_modules():
assert submodule not in self.module_set, f"Don't register module={module} twice"
submodule._module_name = name
Hook._add_module_name(submodule, name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self. ?

@@ -197,6 +198,14 @@ def register_hook(self, module):
# for compatibility with ZCC patches which call this
self.register_module(module)

@staticmethod
def _add_module_name(module, module_name):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason why static?

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.

3 participants