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

Allow static inheritence for ScriptModules #20503

Closed
wants to merge 3 commits into from

Conversation

zdevito
Copy link
Contributor

@zdevito zdevito commented May 14, 2019

Stack from ghstack:

  • #20503 Allow static inheritence for ScriptModules

Summary: Previously, we registered script modules of parent classes during their init methods, making it impossible
to correctly 'override' a method or leave it blank in a subclass. This diff adds the functionality by waiting
until the child init is finished before registering all the methods.

Test Plan: python test/test_jit.py

Differential Revision: D15341555

Summary: Previously, we registered script modules of parent classes during their init methods, making it impossible
to correctly 'override' a method or leave it blank in a subclass. This diff adds the functionality by waiting
until the child init is finished before registering all the methods.

Test Plan: python test/test_jit.py
@pytorchbot pytorchbot added the oncall: jit label May 14, 2019
@zdevito zdevito requested a review from eellison May 14, 2019
Copy link
Contributor

@eellison eellison left a comment

Looks good!

Could you add a test for a module inheritance hierarchy of 3 ? It's not clear to me that we're currently iterating through bases in the right order, since the current test only has a single base.

test/test_jit.py Outdated
v = torch.rand(3, 4)
b = B()
self.assertEqual(b(v), v + v * v)
>>>>>>> Allow static inheritence for ScriptModules
Copy link
Contributor

@eellison eellison May 14, 2019

Choose a reason for hiding this comment

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

looks like merge error here

@TheCodez
Copy link
Contributor

@TheCodez TheCodez commented May 15, 2019

Is it possible to call the super class forward method in the derived class forward method?

@zdevito
Copy link
Contributor Author

@zdevito zdevito commented May 15, 2019

super is not support and it will take some time before we can support it.

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented May 15, 2019

@zdevito merged this pull request in 5243fe0.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented May 15, 2019

@zdevito merged this pull request in 5243fe0.

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

Successfully merging this pull request may close these issues.

None yet

6 participants