Neutone / neutone_sdk

Join the community on Discord for more discussions around Neutone! https://discord.gg/VHSMzb8Wqp
GNU Lesser General Public License v2.1
465 stars 21 forks source link

attribute lookup is not defined on python value of type 'type' #45

Closed robclouth closed 1 year ago

robclouth commented 1 year ago

I can't export any models. Even the simple delay model. I get this error:

RuntimeError: 
attribute lookup is not defined on python value of type 'type':
  File "/usr/local/lib/python3.10/dist-packages/neutone_sdk/sqw.py", line 190
    def calc_delay_samples(io_bs: int, model_bs: int) -> int:
        # TODO(cm): document logic behind this
        saturation_n = SampleQueueWrapper.calc_saturation_n(io_bs, model_bs)
                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ <--- HERE
        return saturation_n - io_bs
'calc_delay_samples' is being compiled since it was called from 'RAVESQW.calc_buffering_delay_samples'
  File "/usr/local/lib/python3.10/dist-packages/neutone_sdk/sqw.py", line 325
    @tr.jit.export
    def calc_buffering_delay_samples(self) -> int:
        delay_samples = self.calc_delay_samples(self.io_bs, self.model_bs)
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ <--- HERE
        if self.is_resampling():
            delay_samples = int(delay_samples * self.daw_bs / self.io_bs)

Any ideas?

christhetree commented 1 year ago

Hi Rob thanks for posting this issue! First off, can you let us know what version of python you're using and post the output of typing 'pip freeze' in the terminal with the python environment you're using for development? Thanks!

christhetree commented 1 year ago

Also does running example_clipper.py give the same error?

robclouth commented 1 year ago

I'm inheriting from SampleQueueWrapper to be able to add a new method. That's where the problem comes from sorry. If I use the original SampleQueueWrapper it works fine. What's the correct way to do this? I want to be able to access a variable inside the WaveformToWaveformBase wrapper.



class RAVESQW(SampleQueueWrapper):
    def __init__(
        self,
        w2w_base: "WaveformToWaveformBase"
    ) -> None:
        super().__init__(w2w_base)

    @tr.jit.export
    def get_preserved_attributes(self) -> List[str]:
        return [
            "forward_bt",
            "is_input_mono",
            "is_output_mono",
            "get_model_name",
            "get_model_authors",
            "get_native_sample_rates",
            "get_native_buffer_sizes",
            "get_wet_default_value",
            "get_dry_default_value",
            "get_default_param_values",
            "get_input_gain_default_value",
            "get_output_gain_default_value",
            "is_resampling",
            "calc_buffering_delay_samples",
            "calc_model_delay_samples",
            "set_daw_sample_rate_and_buffer_size",
            "reset",
            "get_preserved_attributes",
            "to_metadata",
            "w2w_base",
            "get_z"
        ]

    @torch.jit.export
    def get_z(self) -> Optional[Tensor]:
        return self.w2w_base.get_z()
christhetree commented 1 year ago

I see, unfortunately inheritance and Torchscript often don't play well together which is why we avoid it as much as possible and designed the SDK such that the SQW doesn't need to be inherited from, only the w2w wrapper which is much simpler.

It looks like Torchscript may be struggling to call the static saturation method from the SQW superclass now that it's changed. Even though it's ugly, I would try copying the 7 static methods from the SQW into the RAVESQW class to see if that solves the issue. Providing the implementation of get_z() here could also be useful if that's causing it.

We may also improve the SDK in the future by using super() instead of SampleQueueWrapper to call the static methods although I think in the past this may also have caused issues with Torchscript.

robclouth commented 1 year ago

Adding the static methods didn't work. so there's no other way of getting a variable out of the model that's been set in the forward method?

The get_z() method literally just accesses a variable in the base model.

christhetree commented 1 year ago

Getting a variable out of the model shouldn't be the issue since we do that for a lot of the other methods, it's the inheritance of the SQW that's causing issues with torchscript.

Inheriting the SQW is a use case the SDK hasn't been built for (which is why all the examples never subclass the SQW, only the w2w wrapper) and hence hasn't been tested for as vigorously either. I do however have three more suggestions:

  1. Make the RAVESQW class a wrapper around SQW instead of inheriting from SQW, similar to what SQW does with the w2w wrapper. Then it should compile and you can add whatever methods you like. This link may be relevant: https://github.com/pytorch/pytorch/issues/36910#issuecomment-1431453232
  2. Modify the SQW directly instead of subclassing it
  3. Inheritance has been an issue in libtorch for a long time and they keep making improvements. Trying a different version of torch and libtorch might prevent this error.
robclouth commented 1 year ago

I've just copied the whole class and it works now. Thanks!