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

add _partial_ to instantiate api #1905

Merged
merged 7 commits into from
Dec 10, 2021
Merged

Conversation

jieru-hu
Copy link
Contributor

@jieru-hu jieru-hu commented Nov 30, 2021

Closes #1283

add _partial_ support to the instantiation API

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 30, 2021
@jieru-hu jieru-hu marked this pull request as ready for review November 30, 2021 16:31
Comment on lines 21 to 22
def partial_equal(obj1: Any, obj2: Any) -> bool:
obj1, obj2 = _convert_type(obj1), _convert_type(obj2)
Copy link
Collaborator

@Jasha10 Jasha10 Dec 2, 2021

Choose a reason for hiding this comment

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

Many places in this file there is the pattern A == B or partial_equal(A, B).
This could be simplified by moving the A == B part inside of the partial_equal function as below:

Suggested change
def partial_equal(obj1: Any, obj2: Any) -> bool:
obj1, obj2 = _convert_type(obj1), _convert_type(obj2)
def partial_equal(obj1: Any, obj2: Any) -> bool:
if obj1 == obj2:
return True
obj1, obj2 = _convert_type(obj1), _convert_type(obj2)

This would enable simplifying the call-sites of partial_equal.
For example, the expression below on line 30

            if not (obj1[i] == obj2[i] or partial_equal(obj1[i], obj2[i])):
                return False

could then be replaced with this:

            if not partial_equal(obj1[i], obj2[i]):
                return False

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've tested out the change locally to make sure it works. If you agree with the idea, I can push a commit to this PR branch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice!! feel free to push, thanks jasha!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done in bf689d3

Comment on lines 48 to 50
def _call_target(_target_: Callable, *args, **kwargs) -> Any: # type: ignore
def _call_target(_target_: Callable, partial: Optional[bool], *args, **kwargs) -> Any: # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we get away with bool instead of Optional[bool]?
(Same for the signature of the instantiate_node function below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the reason it is None is a bit nuanced. with configs like _recursive_,_convert_, a child node will always inherit the parent node config if the config is not specifically set in the child node. But _partial_ is different, i think it only makes sense to set _partial_ per node, which means if _partial_ is not set in a node, it should default to False instead of inherit that from the parent.

Copy link
Collaborator

@Jasha10 Jasha10 Dec 3, 2021

Choose a reason for hiding this comment

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

I agree that _partial_ should be set per-node...
but why not make the default partial=False instead of partial=None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, Pr updated! :)

Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

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

Removing straggler parentheses from my previous commit:

tests/instantiate/__init__.py Outdated Show resolved Hide resolved
tests/instantiate/__init__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

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

A few more nits:

hydra/_internal/instantiate/_instantiate2.py Outdated Show resolved Hide resolved
hydra/_internal/instantiate/_instantiate2.py Outdated Show resolved Hide resolved
hydra/_internal/instantiate/_instantiate2.py Outdated Show resolved Hide resolved
hydra/_internal/instantiate/_instantiate2.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Jasha10 Jasha10 left a comment

Choose a reason for hiding this comment

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

This PR suffers from the same bug as #1669:

>>> instantiate({"_target_": "builtins.dict", "_partial_": True})
functools.partial(<class 'dict'>)
>>> instantiate({"_target_": "builtins.dict", "_partial_": True, "partial": "unrelated"})
Traceback (most recent call last):
...
TypeError: _call_target() got multiple values for argument 'partial'

@jieru-hu jieru-hu merged commit d674cd2 into facebookresearch:main Dec 10, 2021
jieru-hu added a commit to jieru-hu/hydra that referenced this pull request Feb 3, 2022
jieru-hu added a commit that referenced this pull request Feb 3, 2022
MattiasDC pushed a commit to MattiasDC/hydra that referenced this pull request Apr 1, 2022
@Jasha10
Copy link
Collaborator

Jasha10 commented Apr 5, 2022

@jieru-hu I think we need a news fragment for this.
Edit: done in #2130.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Partial instantiate/call
3 participants