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

Better error messaging for requiring ModuleList to be constant #13899

Closed
sidazhang opened this issue Nov 13, 2018 · 4 comments
Closed

Better error messaging for requiring ModuleList to be constant #13899

sidazhang opened this issue Nov 13, 2018 · 4 comments
Labels
oncall: jit Add this issue/PR to JIT oncall triage queue

Comments

@sidazhang
Copy link

sidazhang commented Nov 13, 2018

🚀 Feature

def __init__
        // ....
        self.conv_lstms = nn.ModuleList(conv_lstms)
        // ...

def forward()
        for i in range(len(self.cell_n)):
            cell = self.conv_lstms[i]
python value of type 'ModuleList' cannot be used as a value:
def forward(self, inputs, hidden):
    for i in range(len(self.cell_n)):
        cell = self.conv_lstms[i]
               ~~~~~~~~~ <--- HERE
        hidden[i] = cell(step_input, hidden[i])
        step_input = hidden[i][0]

    return step_input, hidden

The fix in the above case is to declare __constants__ = ["conv_lstms"]

So the error message should say that ModuleList should be declared constant

Motivation

The error message is very unclear for new users.

@suo suo added the oncall: jit Add this issue/PR to JIT oncall triage queue label Nov 13, 2018
@suo
Copy link
Member

suo commented Nov 23, 2018

Thanks for the feedback! We will look to improve our error messaging in the next release. This issue has been added to our internal triage.

@sto-chastic
Copy link

My code (follows) appends to ModuleList, therefore when I declare ModuleList as constant, I get the error:

AttributeError: _ConstModuleList object has no attribute append

Is there a work arround for this?
Code:

`class resblock(ScriptModule):
constants = ["module_list"]

def __init__(self, ch, nblocks=1, shortcut=True):

    super().__init__()
    self.shortcut = shortcut
    self.module_list = nn.ModuleList()
    for i in range(nblocks):
        resblock_one = nn.ModuleList()
        resblock_one.append(add_conv(ch, ch//2, 1, 1))
        resblock_one.append(add_conv(ch//2, ch, 3, 1))
        self.module_list.append(resblock_one)

@script_method
def forward(self, x):
    for i in range(len(self.module_list)):
        module = self.module_list[i]
        h = x
        for res in module:
            h = res(h)
        x = x + h if self.shortcut else h
    return x`

@sidazhang
Copy link
Author

You need to append the resnet blocks into an array and then initialize the module_list

For example

m = []
m.append(blah)
self.module_list = nn.ModuleList(m)

facebook-github-bot pushed a commit that referenced this issue Feb 15, 2019
Summary:
Add suggestion to add to __constants__ when a ModuleList of Sequential module is used as a tuple

Addresses #13899
Pull Request resolved: #17167

Differential Revision: D14107688

Pulled By: eellison

fbshipit-source-id: 8c07d1f3e25a9c6bdcfd96dbf6b72c2130838278
@eellison
Copy link
Contributor

#17167 suggests add to __constants__ when a module list is used as a tuple

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
oncall: jit Add this issue/PR to JIT oncall triage queue
Projects
None yet
Development

No branches or pull requests

4 participants