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 Python-native logging to Catalyst frontend #660

Merged
merged 11 commits into from
Jun 5, 2024

Conversation

mlxd
Copy link
Member

@mlxd mlxd commented Apr 16, 2024

Before submitting

Please complete the following checklist when submitting a PR:

  • All new functions and code must be clearly commented and documented.

  • Ensure that code is properly formatted by running make format.
    The latest version of black and clang-format-14 are used in CI/CD to check formatting.

  • All new features must include a unit test.
    Integration and frontend tests should be added to frontend/test,
    Quantum dialect and MLIR tests should be added to mlir/test, and
    Runtime tests should be added to runtime/tests.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.


Context: PennyLane's logging support allows developers to explore the execution pipeline end-to-end, while tying into the Python-native logging framework. This has the advantage of being enterprise ready, consumable by a variety of log sinks, and unifies/removes the need for adding print statements throughout a code-base, with all control to be handled by the Python language standard libraries. This PR aims to unify Catalyst to follow the same design as PennyLane to support logging with the same unified configuration, allowing ease of examining of function entry points at the DEBUG level, and a custom TRACE method for expanding functions passed as arguments.

Description of the Change: Adds preliminary support to a variety of Catalyst frontend public API entry-points. Any workload using qml.logging.enable_logging() allows the function entry data to be streamed to the pre-configured log-handlers from PennyLane.

Benefits: More easily allows time-order and standardised data formatting for consumption with complex application execution, debugging, and end-to-end validation of call-points.

Possible Drawbacks:

Related GitHub Issues: PennyLaneAI/pennylane#5528

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.07%. Comparing base (b7ad7db) to head (d2fa7b2).

Current head d2fa7b2 differs from pull request most recent head 18c20fd

Please upload reports for the commit 18c20fd to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #660      +/-   ##
==========================================
+ Coverage   98.05%   98.07%   +0.02%     
==========================================
  Files          69       70       +1     
  Lines        9547     9654     +107     
  Branches      764      762       -2     
==========================================
+ Hits         9361     9468     +107     
  Misses        151      151              
  Partials       35       35              

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

@mlxd mlxd changed the title Add Python-native logging to Catalyst frontend [WIP] Add Python-native logging to Catalyst frontend Apr 17, 2024
@mlxd
Copy link
Member Author

mlxd commented Apr 17, 2024

[sc-61592]

@mlxd mlxd force-pushed the feature/catalyst_frontend_logging branch from 3c4b869 to f5fdbb5 Compare May 31, 2024 14:56
@mlxd mlxd marked this pull request as ready for review May 31, 2024 18:31
@mlxd mlxd changed the title [WIP] Add Python-native logging to Catalyst frontend Add Python-native logging to Catalyst frontend May 31, 2024
@mlxd mlxd requested review from dime10 and erick-xanadu May 31, 2024 18:33
Copy link
Collaborator

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

Looks good to me 💯

Is this a new requirement now for all new functions, classes, and modules going forward?

frontend/catalyst/__init__.py Outdated Show resolved Hide resolved
frontend/catalyst/jit.py Show resolved Hide resolved
Co-authored-by: David Ittah <dime10@users.noreply.github.com>
@mlxd
Copy link
Member Author

mlxd commented Jun 3, 2024

Looks good to me 💯

Is this a new requirement now for all new functions, classes, and modules going forward?

Not a hard requirement, but anything that will be part of the public execution API would be recommended to be added. For now, public API endpoints of PennyLane's execution pipeline and the Catalyst frontend are the ones with additions.

If it will provide benefit (as in transformations, intermediate modifications, etc) it's likely worth adding. For simple I/O, likely can be ignore.

tl;dr discretionary additions are all good.

@mlxd mlxd requested a review from maliasadi June 3, 2024 13:51
Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

Thanks @mlxd! Could you also update the changelog with a user example for this logging support?

.github/workflows/check-catalyst.yaml Show resolved Hide resolved
.github/workflows/check-catalyst.yaml Show resolved Hide resolved
frontend/catalyst/__init__.py Outdated Show resolved Hide resolved
@mlxd mlxd merged commit f83b2c3 into main Jun 5, 2024
37 checks passed
@mlxd mlxd deleted the feature/catalyst_frontend_logging branch June 5, 2024 13:36
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

3 participants