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

Implement qml.breakpoint() and initial integration with Pdb #5600

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Jaybsoni
Copy link
Contributor

@Jaybsoni Jaybsoni commented Apr 30, 2024

Context:
Part of the effort to add more tools for algorithmic debugging of quantum circuits with PennyLane. In this PR we introduce the qml.breakpoint() function which allows users to insert breakpoints into their quantum workflow and enter a debugging context (via Pdb) to step through their quantum functions.

Description of the Change:

  • Added a PLDB class which inherits most of its functionality from Pdb.
  • Added functionality to pass active device information into the debugger during qnode execution to validate the compatibility between the device and the debugger.

image

@Jaybsoni Jaybsoni added the WIP 🚧 Work-in-progress label Apr 30, 2024
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@Jaybsoni
Copy link
Contributor Author

[sc-62243]

@Jaybsoni
Copy link
Contributor Author

Open Questions:

  • Do we want to take any specialized actions when users call step down or step up? Restrict it? Raise a warning? (Currently it is allowed and leads the user into complex pennylane internals)
  • How should we test the breakpoint functionality without triggering the interactive debugger in the test suite? Are there any industry standards here?
  • I have currently added validation checks for incompatible devices (anything other than lightning.qubit and default.qubit) as well is if the breakpoint is called outside of a qnode execution. Any thoughts on this?

@Jaybsoni Jaybsoni changed the title [WIP] Implement qml.breakpoint() and initial integration with Pdb Implement qml.breakpoint() and initial integration with Pdb Apr 30, 2024
@Jaybsoni Jaybsoni added review-ready 👌 PRs which are ready for review by someone from the core team. and removed WIP 🚧 Work-in-progress labels Apr 30, 2024
Args:
dev (Union[Device, "qml.devices.Device"]): The active device
"""
cls.__active_dev.append(dev)
Copy link
Member

Choose a reason for hiding this comment

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

So devices are add-only, not not selectively removed --- as in, we expect them to be appended, and reset the entire list to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly! In fact, instead of appending, we could probably just override the 1st entry in the list each time, since there shouldn't be more than one "active device" at a time.

PLDB.valid_context() # Ensure its being executed in a valid context

debugger = PLDB()
debugger.set_trace(sys._getframe().f_back) # pylint: disable=protected-access
Copy link
Member

Choose a reason for hiding this comment

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

Is this always guaranteed to work? What happens if I do this from the CLI versus a Jupyter notebook cell --- is the correct frame used in both cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know it works for the CLI and from within a Jupyter-notebook cell. Not sure if it will work with other interactive python sessions.

Comment on lines +922 to +925
if PLDB.is_active_dev():
PLDB.reset_active_dev()

PLDB.add_device(self.device)
Copy link
Member

Choose a reason for hiding this comment

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

Just a quick check, but this setup shouldn't have any problem with concurrent execution, right? If we in parallel create 2 qnodes with 2 devices, what happens here if they try to modify the class at the same time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They will most definitely clash! I definitely think this feature should not be used with concurrent execution, I don't know if a user would ever want to actually debug the circuit for 2 qnodes concurrently?

I based this implementation on how the Queuing manger is structured, how does that correspond with concurrent execution

Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.68%. Comparing base (9c9b6ba) to head (f82ea4d).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5600      +/-   ##
==========================================
- Coverage   99.68%   99.68%   -0.01%     
==========================================
  Files         412      412              
  Lines       38666    38447     -219     
==========================================
- Hits        38545    38325     -220     
- Misses        121      122       +1     

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

Copy link
Contributor

@trbromley trbromley left a comment

Choose a reason for hiding this comment

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

Thanks @Jaybsoni!

Comment on lines +113 to +114
class PLDB(pdb.Pdb):
"""Custom debugging class integrated with Pdb."""
Copy link
Contributor

Choose a reason for hiding this comment

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

To confirm, this isn't something users would typically interact with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly! We don't want the user calling the PLDB class at all. In the same way we wouldn't want them to interact with the QueuingManager class for example.



def breakpoint():
"""Launch the custom PennyLane debugger."""
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the plan with docs here - normally PRs should have detailed docs, especially for major user-facing functions like this. Though if we have a story to track a subsequent docs revisit and polish, I'm fine with that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a story to revisit the docs and polish. I will add a little more detail to the breakpoint docs in this PR, but once all of the functionality (measuring states, queuing gates in debug mode, etc.) are added in the subsequent PRs, then we can finalize its docstring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor point that we can discuss in a docs PR:

We don't have a quickstart (.rst) for debugging yet. And it doesn't seem like we make quickstarts for any of the high level files in the pennylane folder. Maybe there's a question here about whether debugging functionality should go in pennylane/debugging.py or in pennylane/debugging/debugging.py

@Jaybsoni Jaybsoni requested review from mlxd and trbromley May 10, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-ready 👌 PRs which are ready for review by someone from the core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants