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

Kwargs seems to not work with classmethods #146

Closed
ahjwang opened this issue Jul 15, 2022 · 6 comments
Closed

Kwargs seems to not work with classmethods #146

ahjwang opened this issue Jul 15, 2022 · 6 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@ahjwang
Copy link

ahjwang commented Jul 15, 2022

🐛 Bug report

First of all, thanks for the great tool! When saving **kwargs in the init of a class which is then used by a classmethod, an assertion error is raised.

To reproduce

from jsonargparse import ArgumentParser


class ClassMethodTest:
    def __init__(self, **kwargs):
        self.kwargs = kwargs
    
    @classmethod
    def get_cls(cls, **kwargs):
        return cls(**kwargs)


class BaseClassWorking:
    def __init__(self, **kwargs):
        self.kwargs = kwargs
        self.classmethod = ClassMethodTest

    def run_clsmethod(self):
        self.classmethod.get_cls(**self.kwargs)


class BaseClassBreaking: 
    def __init__(self, **kwargs):
        self.kwargs = kwargs

    def run_clsmethod(self):
        ClassMethodTest.get_cls(**self.kwargs)


if __name__ == "__main__":
    parser = ArgumentParser()
    parser.add_class_arguments(BaseClassBreaking, "baseclass")
    # parser.add_class_arguments(BaseClassWorking, "baseclass")
    args = parser.parse_args()

Running the script produces the error:

...lib/python3.8/site-packages/jsonargparse/parameter_resolvers.py", line 349, in get_node_component
    assert hasattr(module, node.func.id), f'No attribute {node.func.id!r} in {module}'
AssertionError: No attribute 'cls' in <module '__main__' from 'mwe_failure.py'>

Interestingly, if you comment out parser.add_class_arguments(BaseClassBreaking, "baseclass") above and uncomment parser.add_class_arguments(BaseClassWorking, "baseclass"), the code runs without error.

Environment

  • jsonargparse version: 4.11.0
  • Python version: 3.8.13
  • How jsonargparse was installed: pip install jsonargparse[signatures]
  • OS: Linux

Thank you in advance for taking a look at this :)

@ahjwang ahjwang added the bug Something isn't working label Jul 15, 2022
mauvilsa added a commit that referenced this issue Jul 18, 2022
…ing #146.

- Documented AST resolver support for **kwargs use in property.
@mauvilsa
Copy link
Member

With the latest commit c0de99e the error above in now just logged at debug level. You can see the log messages by setting environment variable JSONARGPARSE_DEBUG=true. Even though now it does not fail, note that neither BaseClassWorking or BaseClassBreaking actually work. I mean if ClassMethodTest.__init__ had some named parameters, these are not resolved. self.classmethod.get_cls(**self.kwargs) is not a supported case as can be seen in the AST resolver documentation. Calling class methods like ClassMethodTest.get_cls(**self.kwargs) is supported, but still it does not work because cls(**kwargs) (using cls analogous to self) is not supported.

Is this pattern used in some popular public library?

@mahnerak
Copy link

Is this pattern used in some popular public library?

@mauvilsa it is! In huggingface
https://github.com/huggingface/transformers/blob/c4cc894086ba86fefbd265f9a80fc8220d2ee182/src/transformers/configuration_utils.py#L540-L558

Because of this I can't use it with Lightning-Transformers library that aims to replace Hydra in favor of LightningCLI (based on jsonargparse) in the future.
I had to create my own classes and manually pass certain keyword arguments instead of **kwargs.

BTW jsonargparse is just awesome! Huge thanks!
I've been using AllenNLP previously for their great configuration framework and I'm happy to see that jsonargparse helps to achieve such experience in Pytorch Lightning as well.

@ahjwang
Copy link
Author

ahjwang commented Jul 18, 2022

Thank you @mauvilsa for your super prompt response! Similar to @mahnerak , the original use case that prompted me to open the issue is using LightningCLI together with lightning-transformers. The base TaskTransformer module in lightning_transformers: has a classmethod config = AutoConfig.from_pretrained(pretrained_model_name_or_path, **self.model_data_kwargs) in the initialize_model() method. So previously when running the following code:

from lightning_transformers.core.model import TaskTransformer
from pytorch_lightning.utilities.cli import LightningCLI

cli = LightningCLI(
    TaskTransformer
)

the following error would be produced.

File ".../jsonargparse/parameter_resolvers.py", line 356, in get_node_component
    assert hasattr(module, node.func.value.id), f'No attribute {node.func.value.id!r} in {module}'
AssertionError: No attribute 'cls' in <module 'transformers.configuration_utils' from '.../transformers/configuration_utils.py'>

Can confirm that with the latest commit the code does not error out.

mauvilsa added a commit that referenced this issue Jul 21, 2022
…146.

- AST resolver now supports pop and get from **kwargs.
- Added AST resolver unit test using class as attribute of module.
@mauvilsa
Copy link
Member

Thank you both for pointing to where this is needed. In recent commits I have added support for cls(**kwargs) and kwargs.pop(...) used in PretrainedConfig of transformers. Not sure if this is all that it is needed to properly support this class. A short check can be done with:

import os
os.environ['JSONARGPARSE_DEBUG'] = ''
from jsonargparse import ArgumentParser
from transformers.configuration_utils import PretrainedConfig

parser = ArgumentParser()
parser.add_class_arguments(PretrainedConfig, 'init')
parser.add_method_arguments(PretrainedConfig, 'get_config_dict', 'get_config_dict')
parser.print_help()

TaskTransformer with LightningCLI I think still will not work because of Type["AutoModel"] in lightning_transformers/core/model.py#L50. Using as type hint Type[] with a forward reference like "AutoModel" is not currently supported. Will look into what needs to be done to support it.

@mahnerak
Copy link

Using as type hint Type[] with a forward reference like "AutoModel" is not currently supported. Will look into what needs to be done to support it.

Yes, I've changed Type[AutoModel] -> Type to make it work.

@mauvilsa
Copy link
Member

Created an issue Lightning-Universe/lightning-transformers#274. Closing this one since anyway the original report has already been fixed and the forward references are out of scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants