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 support for functools.partial in ExternalSource. #3143

Merged
merged 1 commit into from
Jul 12, 2021

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Jul 8, 2021

Signed-off-by: Michał Zientkiewicz mzient@gmail.com

Why we need this PR?

Pick one, remove the rest

  • It fixes a bug: infinite recursion when functools.partial is passed to ExternalSource
  • It adds new feature: recognize partial invocations of generator functions

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

  • What solution was applied:
    • Check whether a callable is functools.partial and if so, use its func field instead to check if it's a generator function
    • Prevent recursion when __call__ field points to self
  • Affected modules and functionalities:
    • External source Python wrapper
  • Key points relevant for the review:
    • N/A
  • Validation and testing:
    • Python test
  • Documentation (including examples):
    • N/A

JIRA TASK: N/A

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
@mzient mzient requested a review from a team July 8, 2021 22:14
@mzient
Copy link
Contributor Author

mzient commented Jul 8, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2563586]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2563586]: BUILD PASSED

Comment on lines +231 to +232
if call == x:
return False
Copy link
Contributor Author

@mzient mzient Jul 9, 2021

Choose a reason for hiding this comment

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

Without the lines 225-227 we got here. Then the functools.partial wrapper ended up having __call__ point to the itself which triggered infinite recursion. This condition prevents this from happening for in other cases. Note that the worst thing that can happen here is that we miss a generator function - much less problematic than crashing on an otherwise good callback.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we miss generator function here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any other cases where call==x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if we miss generator function here?

It won't work as a generator function - so the particular use case is not handled

Are there any other cases where call==x?

None so far, but I'd venture a guess that there might be something implemented in a similar way that functools.partial was.

@@ -222,9 +222,14 @@ def _is_generator_function(x):
where __call__ is a generator function"""
if inspect.isgeneratorfunction(x):
return True
if isinstance(x, functools.partial):
return _is_generator_function(x.func)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JanuszL JanuszL self-assigned this Jul 9, 2021
@mzient mzient merged commit c91caae into NVIDIA:main Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants