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

DM-41711: Upgrade QuantumGraphExecutionReport to handle multiple overlapping graphs #294

Merged
merged 8 commits into from
Sep 23, 2024

Conversation

eigerx
Copy link
Contributor

@eigerx eigerx commented Jun 27, 2024

Depends on lsst/pipe_base#426

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

@eigerx eigerx force-pushed the tickets/DM-41711 branch 2 times, most recently from 705ee46 to a6c4f8d Compare June 27, 2024 23:21
Copy link

codecov bot commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 78.86179% with 26 lines in your changes missing coverage. Please review.

Project coverage is 89.40%. Comparing base (12378ff) to head (53d339a).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
python/lsst/ctrl/mpexec/cli/script/report.py 59.37% 17 Missing and 9 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #294      +/-   ##
==========================================
- Coverage   89.55%   89.40%   -0.15%     
==========================================
  Files          50       50              
  Lines        4317     4427     +110     
  Branches      693      718      +25     
==========================================
+ Hits         3866     3958      +92     
- Misses        325      336      +11     
- Partials      126      133       +7     

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

@eigerx eigerx force-pushed the tickets/DM-41711 branch 5 times, most recently from c4711de to 9ea819a Compare July 17, 2024 19:29
@eigerx eigerx force-pushed the tickets/DM-41711 branch 2 times, most recently from 9f4bd4c to 43b712c Compare August 1, 2024 00:54
@eigerx eigerx marked this pull request as ready for review August 7, 2024 19:08
@eigerx eigerx force-pushed the tickets/DM-41711 branch 4 times, most recently from 78398d1 to ea0b027 Compare September 18, 2024 23:28
Copy link
Contributor

@MichelleGower MichelleGower left a comment

Choose a reason for hiding this comment

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

Mostly suggestions about comments, but some code questions. Merge approved.

help="Collection(s) associated with said graphs/processing."
" For use in `lsst.daf.butler.registry.queryDatasets` if paring down the query would be useful.",
)
@click.option("--where", default="", help="A 'where' string to use to constrain the collections, if passed.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could reuse click options from butler. But help strings are a little different here, so not sure which is better.

Copy link
Member

Choose a reason for hiding this comment

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

You can override the default help if you use a standard option.

" quanta to the screen. Note: the default is to output a yaml file with error information"
" (data_ids and associated messages) to the current working directory instead.",
help="Use the QuantumProvenanceGraph instead of the QuantumGraphExecutionReport, "
"even when there is only one qgraph.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it use v2 automatically when more than one qgraph? If yes, maybe change help message.


REPO is the location of the butler/registry config file.

QGRAPH is the URL to a serialized Quantum Graph file.
QGRAPHS is a `Sequence` of links to serialized Quantum Graphs which have
been executed and are to be analyzed.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why it wasn't this way before, but doesn't this function need the LSST style docstring with PARAMETERS?

Copy link
Member

Choose a reason for hiding this comment

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

No because click formats this string as help text and doesn't understand numpydoc.

butler_config : `str`
The Butler used for this report. This should match the Butler used
for the run associated with the executed quantum graph.
qgraph_uris : `Sequence[str]`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the formatting of this is different, but cannot find exact example. See https://developer.lsst.io/python/numpydoc.html#sequence-types for list example.


with open("delete_me.yaml", "w") as f:
yaml.safe_dump(report_output_dict, f)

Copy link
Contributor

Choose a reason for hiding this comment

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

Debugging lines accidentally left in.

@eigerx eigerx force-pushed the tickets/DM-41711 branch 3 times, most recently from 4f218a2 to 8e7c828 Compare September 21, 2024 04:20
@eigerx eigerx merged commit 6c9b749 into main Sep 23, 2024
13 checks passed
@eigerx eigerx deleted the tickets/DM-41711 branch September 23, 2024 19:45
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.

3 participants