Skip to content

Revert resource tracking to once again print to stdout #7392

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

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

Conversation

jzaia18
Copy link
Contributor

@jzaia18 jzaia18 commented May 7, 2025

Context:
The new resource tracking feature introduced in #7226 logs resource tracking information to a json file. This can cause issues when the file gets clobbered after multiple runs.

Description of the Change:
This PR reverts the behavior of resource tracking to once again print resource information to stdout instead of writing to a json file. This behavior is consistent with Catalyst.

Benefits:
Avoids potential issues with file clobbering.

Possible Drawbacks:
Dirties stdout when resource tracking is enabled, makes parsing output slightly more involved.

Related GitHub Issues:
#7226
[sc-88213]

@jzaia18 jzaia18 requested review from mlxd, albi3ro and maliasadi May 7, 2025 14:37
@jzaia18 jzaia18 self-assigned this May 7, 2025
Copy link
Contributor

github-actions bot commented May 7, 2025

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.

@albi3ro
Copy link
Contributor

albi3ro commented May 7, 2025

If we're running into issues with the file getting too much stuff in it, could we just log each run to a different file by allowing the file to be set on the device at initialization? I can't see how writing to stdout would keep things cleaner than a file.

Copy link

codecov bot commented May 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.65%. Comparing base (e21660f) to head (480fd93).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7392   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files         529      529           
  Lines       50610    50615    +5     
=======================================
+ Hits        50434    50439    +5     
  Misses        176      176           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

🙌

Copy link
Contributor

@AntonNI8 AntonNI8 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, pending existing suggestions👍

@josh146
Copy link
Member

josh146 commented May 8, 2025

I haven't been involved in the discussions here, so apologies if this was already discussed, but is there not a way here to support structured and/or configurable output from null.qubit?

E.g., an option to null.qubit for users to provide the print behaviour (stdout, logger, writing to a file, etc).

@jzaia18
Copy link
Contributor Author

jzaia18 commented May 8, 2025

I could do this but it would likely involve adding another parameter to the constructor. Alternatively I could replace track_resources as a bool with something like (None => do not track, file-like => use the provided object, str => create a file with this name). Thoughts? @josh146 @albi3ro @maliasadi

@josh146
Copy link
Member

josh146 commented May 8, 2025

@jzaia18 I'll leave this to others that are involved in this project, but I can imagine as a user common use cases will be wanting to perform computations/visualizations on the data, and it can be non-trivial to do this programmatically if the data is on stdout!

As a python user I would instead want to work with structured data of some sort (dicts, np arrays, etc)

@AntonNI8
Copy link
Contributor

AntonNI8 commented May 8, 2025

As a python user I would instead want to work with structured data of some sort (dicts, np arrays, etc)

I'm not concerned with what's end-user-facing here, but it's true that the flexibility could be helpful internally.
The default to stdout to avoid JSON files blowing up, and for consistency with Catalyst makes sense.
Adding another parameter to the constructor seems better than trying to encapsulate all of the details in track_resources.

@jzaia18
Copy link
Contributor Author

jzaia18 commented May 8, 2025

I have a working prototype that takes the following approach. track_resources may be one of (bool, None, str, file-like). If False or None, resource tracking is disabled. If True, it writes to stdout. If given a filename as a str, it creates and writes to the passed file. And I don't advertise this in the docstring because I can't easily support this with Catalyst, but if you pass a file-like object, it can write to that too. All other options I was able to test and get working with Catalyst. I think this may be the closest to a solution that satisfies everybody, and it should make my hook in the benchmarks repo more streamlined as well. What do you all think? @maliasadi @AntonNI8 @josh146

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