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 source to be a decorated function or method #5268

Merged
merged 8 commits into from
Jan 18, 2024

Conversation

klecki
Copy link
Contributor

@klecki klecki commented Jan 5, 2024

Category: Bug fix

Description:

This changes how the external source counts the number of arguments that the source callback uses.
Instead of relying on the __code__.co_argcount we use inspect.signature.
The reason is that a decorator can (and usually does) propagate the signature, but due to the use of
generic *args, **kwargs parameters it will hide the original argcount of the original function that is wrapped.

Such simple decorator is added as a test case, before this change the following code won't work.

This fixes the potential error of DALI assuming that there are mandatory arguments to the function,
that doesn't expect them (or vice-versa) and invoking the callback with (or without) passing the source_info.
The error would point to the callback invocation and would not indicate the decorators at all.

Now we can sensibly error out on:

  • var-args
  • var-kwargs
  • kwarg-only argument

There is no need to differentiate between positional only and named argument, if we first detect
any disallowed constructs. Note, that sources like foo(arg, *args) were already disallowed, but the reported arg count was something like -1 or -2 by mistake rather than by design.

Next we can validate the correct number of arguments.

The detection of callbacks was done inside an exception handler, and throwing another exception
from there would chain the exception, so the construction of argument description is moved to line below.

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: N/A

@klecki klecki marked this pull request as draft January 5, 2024 17:11
Copy link
Contributor

@mzient mzient left a comment

Choose a reason for hiding this comment

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

We can fix (and simplify!) autograph do_not_convert instead of building workarounds in DALI, which is just kicking the can down the road.
See #5270

@mzient mzient self-requested a review January 8, 2024 08:57
Copy link
Contributor

@mzient mzient left a comment

Choose a reason for hiding this comment

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

We can do it for the sake of different decorators than do_not_convert.

@klecki klecki marked this pull request as ready for review January 16, 2024 16:57
@klecki
Copy link
Contributor Author

klecki commented Jan 16, 2024

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12134625]: BUILD STARTED

@klecki
Copy link
Contributor Author

klecki commented Jan 16, 2024

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12134931]: BUILD STARTED

@stiepan stiepan self-assigned this Jan 16, 2024
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12134931]: BUILD FAILED

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki
Copy link
Contributor Author

klecki commented Jan 17, 2024

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12153458]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12153458]: BUILD PASSED

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>
@klecki
Copy link
Contributor Author

klecki commented Jan 17, 2024

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12162623]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12162623]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12162623]: BUILD PASSED

@klecki klecki merged commit 86a270d into NVIDIA:main Jan 18, 2024
7 checks passed
@klecki klecki deleted the use-signature-in-callbacks branch January 18, 2024 16:16
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.

5 participants