PennyLaneAI / catalyst

A JIT compiler for hybrid quantum programs in PennyLane
https://docs.pennylane.ai/projects/catalyst
Apache License 2.0
122 stars 27 forks source link

[Frontend] Nice identifiers for callbacks in the IR #919

Closed erick-xanadu closed 1 month ago

erick-xanadu commented 1 month ago

Context: Instead of a numeric identifier, callbacks now include the python attribute __name__ in their IR.

Description of the Change: Cleaning up a little bit of code, made AnnotatedFunction class to avoid using many closures, used functools.wraps and functools.update_wrapper where necessary. As an important note, we do not use the __module__ key in wraps nor update_wrapper. Otherwise, autograph would transform the functions returned from callbacks leading to errors.

Benefits: Easier debugging in the IR.

Possible Drawbacks: None.

Related GitHub Issues:

[sc-68128]

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.97%. Comparing base (4018d04) to head (f5c36c4).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #919 +/- ## ======================================= Coverage 97.97% 97.97% ======================================= Files 71 71 Lines 10476 10493 +17 Branches 956 956 ======================================= + Hits 10264 10281 +17 Misses 169 169 Partials 43 43 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rauletorresc commented 1 month ago

The PR does more than what it says it does: Nice identifiers for callbacks. This makes it difficult to actually focus on the relevant changes, and even harder to evaluate the additions.

erick-xanadu commented 1 month ago

@rauletorresc all these changes were necessary for the nice identifiers. Otherwise the __name__ field would be different. It may have been closure or wrapper. Depending on the closure that was sent as a callback.

rauletorresc commented 1 month ago

@rauletorresc all these changes were necessary for the nice identifiers. Otherwise the __name__ field would be different. It may have been closure or wrapper. Depending on the closure that was sent as a callback.

I understand they are necessary but having them in separated PRs would have eased the review for me. All fine!

rauletorresc commented 1 month ago

For example: "made AnnotatedFunction class to avoid using many closures" would be a good candidate for a PR.

rauletorresc commented 1 month ago

LGTM!