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

Callable type hint does not work for callable classes #110

Closed
FeryET opened this issue Dec 8, 2021 · 4 comments
Closed

Callable type hint does not work for callable classes #110

FeryET opened this issue Dec 8, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@FeryET
Copy link

FeryET commented Dec 8, 2021

Hi.

I'm trying to typehint my project accordingly, and I have something like this:

# module.py
class A:
    def __call__(self):
         pass

class B:
   def __init__(self, arr: Sequence[Callable]):
        self.arr = arr 

I have this config file:

# config.yaml
item_b:
     arr: 
       - class_path: module.A
       - class_path: module.A
       - class_path: module.A

But this will lead to some incomprehensible errors that will finally show that

Configuration check failed :: Parser key "item_b.arr": Not of type typing.Callable. Expected a dot import path string

This is actually a reproduction of a code I'm trying to implement using PytorchLightning's new CLI, but I think the issue comes from jsonargparse.

@mauvilsa
Copy link
Member

mauvilsa commented Dec 9, 2021

The support for Callable currently is only experimental, see https://jsonargparse.readthedocs.io/en/stable/#type-hints. In particular callable classes are not supported. For now you can only provide import paths to functions.

Class A is not a callable. The instances of class A are callable. A type hint that would work now would be Sequence[A] meaning a sequence of instances of any subclass of A, i.e.:

from typing import Sequence
from jsonargparse import ArgumentParser, ActionConfigFile

class A:
    def __call__(self):
        print('A called')

class B:
   def __init__(self, arr: Sequence[A]):
        self.arr = arr 

parser = ArgumentParser()
parser.add_class_arguments(B, 'item_b')
parser.add_argument('--config', action=ActionConfigFile)

config="""
item_b:
  arr: 
    - class_path: __main__.A
"""

cfg = parser.parse_args([f'--config={config}'])
print(cfg.item_b)  # Namespace(arr=[Namespace(class_path='__main__.A')])

I don't see in the python standard library a generic CallableClass type which would make sense in this case. CallableClass could implemented as an abstract class, but then all callable classes would need to inherit from it. So it is the same situation as A used as a base class.

The jsonargparse implementation of Callable type could be extended such that if it is given a dict with class_path then it acts as what you expected. So I mark this as an enhancement.

@mauvilsa mauvilsa added the enhancement New feature or request label Dec 9, 2021
@mauvilsa mauvilsa changed the title jsonargparse can only rely on concrete classes. Callable type hint does not work for callable classes Dec 9, 2021
@FeryET
Copy link
Author

FeryET commented Dec 9, 2021

The support for Callable currently is only experimental, see https://jsonargparse.readthedocs.io/en/stable/#type-hints. In particular callable classes are not supported. For now you can only provide import paths to functions.

Class A is not a callable. The instances of class A are callable. A type hint that would work now would be Sequence[A] meaning a sequence of instances of any subclass of A, i.e.:

from typing import Sequence
from jsonargparse import ArgumentParser, ActionConfigFile

class A:
    def __call__(self):
        print('A called')

class B:
   def __init__(self, arr: Sequence[A]):
        self.arr = arr 

parser = ArgumentParser()
parser.add_class_arguments(B, 'item_b')
parser.add_argument('--config', action=ActionConfigFile)

config="""
item_b:
  arr: 
    - class_path: __main__.A
"""

cfg = parser.parse_args([f'--config={config}'])
print(cfg.item_b)  # Namespace(arr=[Namespace(class_path='__main__.A')])

I don't see in the python standard library a generic CallableClass type which would make sense in this case. CallableClass could implemented as an abstract class, but then all callable classes would need to inherit from it. So it is the same situation as A used as a base class.

The jsonargparse implementation of Callable type could be extended such that if it is given a dict with class_path then it acts as what you expected. So I mark this as an enhancement.

Thank you for your detailed answer! But I think you're a bit wrong about Callables. Feel free to correct me if I'm wrong here but I thinkg my implementation and typehinting is correct (although there is a serious lack of documentation about magic functions in python). Python infers that any class that has implemented __call__ is a callable. See this and this issue. The problem is that I really do not like arbitrary inheritance that really isn't that meaningful. Currently, I have created an abstract base class that has only an abstract __call__ method. But this also gives way to other issues (a constant need to create baseclasses, the issues of changing the call signature for some functions and etc.)

@mauvilsa
Copy link
Member

mauvilsa commented Dec 9, 2021

But I think you're a bit wrong about Callables.

I guess you saw an email notification and read my initial comment where questioned if your type hint was wrong. I changed that quickly. Sorry about that.

Your type hint is correct and your use of class_path is consistent with the way jsonargparse works. But this is not yet supported for the Callable type. This is why I added:

The jsonargparse implementation of Callable type could be extended such that if it is given a dict with class_path then it acts as what you expected. So I mark this as an enhancement.

mauvilsa added a commit that referenced this issue Mar 22, 2022
- Fixed bug in check for class_path, init_args dicts.
- Fixed module mocks in cli_tests.py.
- Callable no longer a simple registered type.
- Import paths are now serialized as its shortest form.
- Some related code cleanup and some minor unrelated fixes.
@mauvilsa
Copy link
Member

@FeryET this is now implemented in commit 9c9dcd2 and will be included in the next release due soon. The feature can be tested for your example with the following:

from typing import Callable, Sequence
from jsonargparse import ArgumentParser, ActionConfigFile

class A:
    def __call__(self):
        print('A called')

class B:
   def __init__(self, arr: Sequence[Callable]):
        self.arr = arr 

parser = ArgumentParser(error_handler=None)
parser.add_class_arguments(B, 'item_b')
parser.add_argument('--config', action=ActionConfigFile)

config="""
item_b:
  arr: 
    - class_path: __main__.A
"""

cfg = parser.parse_args([f'--config={config}'])
print(cfg.item_b)
cfg_init = parser.instantiate_classes(cfg)
print(cfg_init.item_b.arr)

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

No branches or pull requests

2 participants