Added super init to Module #91819

Vivswan · 2023-01-06T19:30:37Z

Added super init to Module for complex user modules derived from multiple python classes.
And by adding the super init call at the end so it doesn't change any functionality of Module class.

I am working on building a module for simulating analog neural network on PyTorch.
and this small change is really useful for that and we can definitely think of many other useful cases especially for more module or mro hierarchy.

Issues: #28746, #48626, #61662, #74036

Added super init to Module for user modules derived from multiple python classes

pytorch-bot · 2023-01-06T19:30:40Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/91819

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

❌ 6 Failures

As of commit 77cdda8:

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following jobs failed but were present on the merge base 438f12d:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

linux-foundation-easycla · 2023-01-06T19:30:41Z

The committers listed above are authorized under a signed CLA.

✅ login: Vivswan / name: Vivswan Shah (ec46af7)

albanD

We tried this before and it was reverted: #74036
I can check what is the status now.

facebook-github-bot · 2023-01-11T18:12:25Z

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

Vivswan · 2023-01-11T18:58:54Z

I see (#74036).

I did first try to check what happens when I put the super().__init__() at the beginning of the nn.Module, in that case it does causes a lot of error, I think that was the reason for revert of #74096 as well. That why I added it at the end to not cause any functionally issue with nn.Module and other module dependent on it

I also understand your opinion on providing args and kwargs to the super class.
Till we figure out the proper functionality to do so.

We can probably still add super().__init__() without arguments which will provide usefulness to majority of the developers dealing with this issue.

albanD

In general, this is BC-breaking for anyone that is currently doing mixin of nn.Module and working around the lack of super() init() manually...
I guess that is ok to break as it is an explicit error

torch/nn/modules/module.py

Vivswan · 2023-01-11T21:06:25Z

if approved, then it can be in the docs of nn.Module as an important notice that now it does call super.__init__(*,**)

OR

to help everyone with huge library's and any error due to this change.
we can provide an option with which they can stop the call to super().__init__(*,**).
this option can be deprecated in the future.

Something maybe like this:

class Module:
    _call_super: bool = True #deprecated in future
    
    def __init__(self, *args, **kwargs) -> None:
        ......
        ......
        if Module._call_super: 
            super().__init__(*args, **kwargs)

in this way anyone can disable the super().__init__(*,**) just by

import torch
torch.nn.Module._call_super = False

Let me know if you want me to implement this. 🙂

albanD · 2023-01-11T21:18:02Z

I like this idea of having a class attribute to control this behavior as a way to make the transition smoother.

Could you add this new class attribute and make it False by default.
That will make this PR not break backward compatibility but allow you to enable this if you need this.

We can do a follow up, BC-breaking, PR that switch the default behavior to True.

default value for call_super_init is False

Vivswan · 2023-01-11T21:42:07Z

Added call_super_init: bool = False to nn.Module

torch/nn/modules/module.py

Signed-off-by: Vivswan Shah <58091053+Vivswan@users.noreply.github.com>

albanD

The change looks good to me. Could you just add a test in test/test_nn.py to make sure it does the right thing when a subclass sets this flag to True please and then we can merge this!

facebook-github-bot · 2023-01-12T22:55:41Z

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

Vivswan · 2023-01-18T00:42:41Z

Can you give me a link to documentation, how to create tests for pytorch?

albanD · 2023-01-18T01:50:47Z

Ho I'm not sure we have doc for that :/
You can add a new function here:

pytorch/test/test_nn.py

Line 165 in cf5a40c

self.assertEqual(m(input).size(), (2, 5))

Call it test_module_super_init and check that you do get the behavior you want.

albanD · 2023-01-20T20:49:34Z

The test failures are relevant. Not sure why at a glance...

Vivswan · 2023-01-21T05:36:01Z

All the failed test are coming from the following code, since now we do accept *args, **kwargs in nn.Module.

pytorch/test/test_module_init.py

Lines 416 to 419 in 6f1727b

    
           # == Verify passing a nonexistent arg errors out. == 
        
           if check_nonexistent_arg: 
        
               with test_cls.assertRaises(TypeError): 
        
                   m = module_cls(*args, **kwargs, nonexistent_arg='foo')

Either we can change this test
OR
We can throw a RuntimeError if args or kwargs are not empty and call_super_init is false.

albanD · 2023-01-24T11:16:40Z

Ho that is interesting haha
Raising an error when call_super_init is False sounds good.
If you could raise the same error (as much as possible, including type and messagee) that is raised today that would be best so that we don't have to update the tests (and thus reduce the chance of breaking user code).

facebook-github-bot · 2023-02-01T16:30:40Z

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

albanD · 2023-02-01T16:30:56Z

Looks good!
Just double checking internally to make sure this doesn't get reverted again and I'll merge it!

albanD · 2023-02-01T18:42:53Z

@pytorchbot merge

pytorchmergebot · 2023-02-01T18:44:43Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2023-02-01T21:56:39Z

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / win-vs2019-cuda11.6-py3 / test (default, 3, 5, windows.g5.4xlarge.nvidia.gpu)

Details for Dev Infra team

Raised by workflow job

albanD · 2023-02-01T22:13:11Z

@pytorchbot merge -f "Flaky test"

pytorchmergebot · 2023-02-01T22:17:55Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Added super init to Module

ec46af7

Added super init to Module for user modules derived from multiple python classes

Vivswan requested review from albanD and jbschlosser as code owners January 6, 2023 19:30

pytorchbot added the open source label Jan 6, 2023

albanD reviewed Jan 11, 2023

View reviewed changes

torch/nn/modules/module.py Outdated Show resolved Hide resolved

added args, kwargs to __init__

e60b22d

added call_super_init

5e44d56

default value for call_super_init is False

albanD reviewed Jan 11, 2023

View reviewed changes

torch/nn/modules/module.py Outdated Show resolved Hide resolved

Vivswan added 2 commits January 11, 2023 21:10

Module to self.call_super_init

03b24eb

to follow the convention

3f5049a

Vivswan added a commit to Vivswan/AnalogVNN that referenced this pull request Jan 12, 2023

added resolution to pytorch/pytorch#91819

181f205

Signed-off-by: Vivswan Shah <58091053+Vivswan@users.noreply.github.com>

drisspg added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 12, 2023

lint: removed trailing spaces

8376d9e

albanD reviewed Jan 12, 2023

View reviewed changes

added test_module_super_init

0258b3b

added TypeError for BC

4e475fd

Vivswan added 2 commits January 24, 2023 12:38

corrected line width

7fe1b15

Merge branch 'pytorch:master' into patch-1

77cdda8

albanD approved these changes Feb 1, 2023

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 1, 2023

pytorchmergebot added the Merged label Feb 1, 2023

pytorchmergebot closed this in 8c1ee89 Feb 1, 2023

Vivswan deleted the patch-1 branch February 4, 2023 18:18

Added super init to Module #91819

Added super init to Module #91819

Uh oh!

Conversation

Vivswan commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/91819

❌ 6 Failures

Uh oh!

linux-foundation-easycla bot commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 11, 2023

Uh oh!

Vivswan commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Vivswan commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albanD commented Jan 11, 2023

Uh oh!

Vivswan commented Jan 11, 2023

Uh oh!

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 12, 2023

Uh oh!

Vivswan commented Jan 18, 2023

Uh oh!

albanD commented Jan 18, 2023

Uh oh!

albanD commented Jan 20, 2023

Uh oh!

Vivswan commented Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albanD commented Jan 24, 2023

Uh oh!

facebook-github-bot commented Feb 1, 2023

Uh oh!

albanD commented Feb 1, 2023

Uh oh!

albanD commented Feb 1, 2023

Uh oh!

pytorchmergebot commented Feb 1, 2023

Merge started

Uh oh!

pytorchmergebot commented Feb 1, 2023

Merge failed

Uh oh!

albanD commented Feb 1, 2023

Uh oh!

pytorchmergebot commented Feb 1, 2023

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Vivswan commented Jan 6, 2023 •

edited

Loading

pytorch-bot bot commented Jan 6, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jan 6, 2023 •

edited

Loading

Vivswan commented Jan 11, 2023 •

edited

Loading

Vivswan commented Jan 11, 2023 •

edited

Loading

Vivswan commented Jan 21, 2023 •

edited

Loading