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

feat: support for partialmethod #665

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MiguelMonteiro
Copy link
Contributor

@MiguelMonteiro MiguelMonteiro commented Jan 30, 2025

What does this PR do?

Adds support for functools.partial and functools.partialmethod and closes #664
Still need to write tests and update documentation but would like wait for @mauvilsa to weigh in to see if the idea makes sense.

Before submitting

  • [ x] Did you read the contributing guideline?
  • Did you update the documentation? (readme and public docstrings)
  • Did you write unit tests such that there is 100% coverage on related code? (required for bug fixes and new features)
  • Did you verify that new and existing tests pass locally?
  • Did you make sure that all changes preserve backward compatibility?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

Copy link

codecov bot commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (eee8ea9) to head (66b7ef6).
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #665   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         6654      6670   +16     
=========================================
+ Hits          6654      6670   +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mauvilsa mauvilsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a reason why not to support this. Please go ahead and write the unit tests.

@MiguelMonteiro
Copy link
Contributor Author

I can't think of a reason why not to support this. Please go ahead and write the unit tests.

I was able to add support for partialmethod and added a test for it. For partial functions and classes it's a bit trickier since they are dynamic and it seems the parameter resolver is expecting static objects for the most part.

The options would be to add a subsection in get_component_and_parent that checks if the function_or_class is a partial and handle it separately? Or change the way the attr is obtained form statically attr = inspect.getattr_static(get_generic_origin(function_or_class), method_or_property) to dynamically. But I don't fully understand the downstream effects of these changes.

Copy link
Member

@mauvilsa mauvilsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to add support for partialmethod and added a test for it. For partial functions and classes it's a bit trickier since they are dynamic and it seems the parameter resolver is expecting static objects for the most part.

What was the reason you came to know that partials didn't work? I would guess it was because you needed it for some use case. If it was only for partialmethod, then what is implemented now already works and is a new feature. If you really want partial to also work, later I can take a look.

jsonargparse_tests/test_parameter_resolvers.py Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
@MiguelMonteiro
Copy link
Contributor Author

I was able to add support for partialmethod and added a test for it. For partial functions and classes it's a bit trickier since they are dynamic and it seems the parameter resolver is expecting static objects for the most part.

What was the reason you came to know that partials didn't work? I would guess it was because you needed it for some use case. If it was only for partialmethod, then what is implemented now already works and is a new feature. If you really want partial to also work, later I can take a look.

For classes the main reason is that partial returns an instance of <class 'functools.partial'> which is not a subclass of the original class. This breaks type hints and detecting subclasses as far as I could tell. Additionally, for both Callables and classes, partial returns a descriptor, hiding the wrapped object under __get__,. This requires changes to be made in the parameter resolution at least, but I can't tell if there are other places which would need to be changed to handle descriptors.

For reference, my use case was to create partial classes. I got around the limitations by using partialmethod and explicitly creating a new type (instead of the original type wrapped in partial):

cls = type(
        name,
        (base_class,),
        {
            "__init__": partialmethod(base_class.__init__, **keywords),
            "__doc__": base_class.__doc__,
        },
    )

@MiguelMonteiro MiguelMonteiro changed the title feat: support for partial and partialmethod feat: support for partialmethod Feb 3, 2025
@@ -899,6 +902,13 @@ def test_get_params_some_ignored():
assert_params(get_params(func_given_kwargs), ["p", "p1"], help=False)


def test_partialmethod():
ClassA.partial_method_a = partialmethod(ClassA.method_a, pma1=1, pma2=0.5)
assert_params(get_params(ClassA, "partial_method_a"), ["pma1", "pma2", "kma1"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking again at this test now I think there are things missing. The partial doesn't change the list of parameter names. But it does change the defaults. Are the defaults correctly inferred by a parser? If not, then it doesn't really work as expected. So this should be tested.

And another thing that could be considered missing is that a method could be set as default to a callable. For this --print_config should serialize this default into an import path string. And this import path when used in a config, should be correctly deserialize into the partial method object.

@mauvilsa
Copy link
Member

mauvilsa commented Feb 3, 2025

For partial functions and classes being dynamic, the import path would also be a problem. If a partial function is saved at the top level of a module in a variable, from the partial function object alone can it be determined what its import path should be?

@mauvilsa mauvilsa added the enhancement New feature or request label Feb 4, 2025
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

Successfully merging this pull request may close these issues.

Add support for partialmethods and partial types
2 participants