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

Allow class_path to be a classmethod. #309

Closed
ordabayevy opened this issue Jun 23, 2023 · 5 comments
Closed

Allow class_path to be a classmethod. #309

ordabayevy opened this issue Jun 23, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@ordabayevy
Copy link

ordabayevy commented Jun 23, 2023

🚀 Feature request

Allow class_path to be a classmethod.

Motivation

It is common to use a classmethod as an alternative way to create new instances:

class Calendar:
    def __init__(self, firstweekday: int):
        self.firstweekday = firstweekday

    @classmethod
    def from_str(cls, firstweekday: str):
        if firstweekday == "Monday":
            return cls(1)
        ...

class MyClass:
    def __init__(self, calendar: Calendar):
        self.calendar = calendar

parser = ArgumentParser()
parser.add_class_arguments(MyClass, "myclass")  

cfg = parser.parse_path("config.yaml")

Then config file would look as follows:

myclass:
  calendar:
    class_path: __main__.Calendar.from_str
    init_args:
      firstweekday: "Monday"

Pitch

A hacky way I tried to make this work is by adding the following to typehints.py file:

    # Subclass
    elif not hasattr(typehint, "__origin__") and inspect.isclass(typehint):
        if isinstance(val, typehint):
            if serialize:
                val = serialize_class_instance(val)
            return val
        if serialize and isinstance(val, str):
            return val

        val_input = val
        val = subclass_spec_as_namespace(val, prev_val)
        if not is_subclass_spec(val):
            raise_unexpected_value(
                f"Not a valid subclass of {typehint.__name__}. Got value: {val_input}\n"
                "Subclass types expect one of:\n"
                "- a class path (str)\n"
                "- a dict with class_path entry\n"
                "- a dict without class_path but with init_args entry (class path given previously)"
            )

        try:
            val_class = import_object(resolve_class_path_by_name(typehint, val["class_path"]))
+           if hasattr(val_class, "__self__"):  # if val_class is a classmethod
+               val_class = val_class.__self__
            if not is_subclass(val_class, typehint):
                raise_unexpected_value(
                    f'Import path {val["class_path"]} does not correspond to a subclass of {typehint}'
                )
            val["class_path"] = get_import_path(val_class)
            val = adapt_class_type(val, serialize, instantiate_classes, sub_add_kwargs, prev_val=prev_val)
        except (ImportError, AttributeError, AssertionError, ArgumentError) as ex:
            class_path = val if isinstance(val, str) else val["class_path"]
            error = indent_text(str(ex))
            raise_unexpected_value(f"Problem with given class_path {class_path!r}:\n{error}", exception=ex)

    return val

Alternatives

Subclassing to allow alternative initialization:

class CalendarFromStr(Calendar):
    def __init__(self, firstweekday: str):
        if firstweekday == "Monday":
            super().__init__(1)
        ...
@ordabayevy ordabayevy added the enhancement New feature or request label Jun 23, 2023
@mauvilsa
Copy link
Member

Welcome and thank you for the proposal!

Apart from the sub-classing alternative you mentioned, note that there is also class_from_function. With just one line you can get a class that has as __init__ the parameters of the @classmethod.

Directly supporting class methods could be added. But, since there is the simple previously mentioned alternative, this is not a high priority. Also, if support for this is implemented, very likely it will not be using class_path. It is inconsistent to provide the path to a method when the key class_path implies that you are supposed to give the path to a class.

@ordabayevy
Copy link
Author

ordabayevy commented Jun 27, 2023

Thank you for your reply!

I thought about using class_from_function but it is not clear to me how. I know how to use class_from_function with MyClass in the example above:

parser = ArgumentParser()
dynamic_class = class_from_function(instantiate_myclass)
parser.add_class_arguments(dynamic_class, "myclass")

However, I don't know how to apply it to Calendar which is an init arg of MyClass and processed automatically:

parser = ArgumentParser()
parser.add_class_arguments(MyClass, "myclass")
# how to use class_from_function with Calendar?

so that I can use Calendar.from_str in my config file.

@mauvilsa
Copy link
Member

I tried it, and actually it doesn't work. What I had in mind was:

CalendarFromStr = class_from_function(Calendar.from_str, Calendar)

And in the config:

myclass:
  calendar:
    class_path: __main__.CalendarFromStr

However, this fails with:

Unexpected import path format: jsonargparse._util.class_from_function.<locals>.ClassFromFunction

I will think about how to fix this.

@mauvilsa
Copy link
Member

With pull request #315 what I had in mind would be possible. Please try it out. The use of class_from_function for the calendar example above would be:

class_from_function(Calendar.from_str, Calendar, name="CalendarFromStr")

@ordabayevy
Copy link
Author

It works. Thank you very much for the fix!

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