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

Cannot override Callable init_args without passing the class_path #174

Closed
tshu-w opened this issue Oct 8, 2022 · 6 comments · Fixed by #175 or #176
Closed

Cannot override Callable init_args without passing the class_path #174

tshu-w opened this issue Oct 8, 2022 · 6 comments · Fixed by #175 or #176
Labels
bug Something isn't working

Comments

@tshu-w
Copy link
Contributor

tshu-w commented Oct 8, 2022

🐛 Bug report

Unlike the class argument, I cannot override Callable argument without passing the class_path even it is given previously.

To reproduce

from jsonargparse import CLI
from typing import Callable

def call():
    print("Hi")

class Call:
    def __init__(self, name: str):
        self.name = name

    def __call__(self):
        print(f"Hi, {self.name}")

def fn(
    callable: Callable = call,
):
    callable()

if __name__ == "__main__":
    CLI(fn)
callable:
  class_path: __main__.Call
  init_args:
    name: Alice
python test.py --config test.yaml
Hi, Alice
python test.py --config test.yaml --callable.name Bob
usage: test.py [-h] [--config CONFIG] [--print_config [={comments,skip_null,skip_default}+]] [--callable CALLABLE]
test.py: error: Unrecognized arguments: --callable.name Bob

Expected behavior

python test.py --config test.yaml
Hi, Alice
python test.py --config test.yaml --callable.name Bob
Hi, Bob

Environment

  • jsonargparse version: 4.15.1
  • Python version: 3.9
  • How jsonargparse was installed: pip install jsonargparse
  • OS (e.g., Linux): macOS & Linux
@mauvilsa
Copy link
Member

I created a pull request #175. Can you please test and review.

@tshu-w
Copy link
Contributor Author

tshu-w commented Oct 14, 2022

I confirmed #175 fixed this issue.

@tshu-w
Copy link
Contributor Author

tshu-w commented Oct 14, 2022

Perhaps it would be better for is_callable_typehint to decide whether to check all subtypes as is_subclass_typehint does.

If I understand correctly, this will make Union[Callable, ...] works either.

@staticmethod
def is_callable_typehint(typehint):
typehint_origin = get_typehint_origin(typehint)
return typehint_origin in callable_origin_types or typehint in callable_origin_types

ActionTypeHint.is_subclass_typehint(typehint, all_subtypes=False) or
ActionTypeHint.is_callable_typehint(typehint) or

@tshu-w tshu-w reopened this Oct 14, 2022
@mauvilsa
Copy link
Member

Since you commented here, I thought you were not going to review the pull request, so I merged it. Your comment is spot on. Would you like to create a pull request with the changes?

@tshu-w
Copy link
Contributor Author

tshu-w commented Oct 14, 2022

Sorry, I didn't realize it when I reviewed the code the first time.

Would you like to create a pull request with the changes

Sure, do you think it's necessary to add another test about this?

@mauvilsa
Copy link
Member

Sure, do you think it's necessary to add another test about this?

Yes, would be good to have a separate test for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants