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-36360: Improve diagnostic visibility for empty QGs. #204

Merged
merged 2 commits into from Sep 30, 2022

Conversation

TallJimbo
Copy link
Member

@TallJimbo TallJimbo commented Sep 27, 2022

By moving the exception raise down to the outer click layer (returning None through all higher layers) we avoid the long traceback that hides the useful information being logged by pipe_base.

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes (in pipe_base)

I expect the build_and_test check here to fail until the pipe_base is branch is merged.

@TallJimbo TallJimbo force-pushed the tickets/DM-36360 branch 4 times, most recently from cc8b79f to ffcda23 Compare September 27, 2022 19:59
Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

Wording suggestion on the exception message, but otherwise this looks good.

@@ -137,7 +137,8 @@ def qgraph(ctx: click.Context, **kwargs: Any) -> None:
file=sys.stderr,
)
return
script.qgraph(pipelineObj=pipeline, **kwargs, show=show)
if script.qgraph(pipelineObj=pipeline, **kwargs, show=show) is None:
raise click.ClickException("QuantumGraph was empty; diagnostic logging above may have details.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of "diagnostic logging above", maybe be more explicit: "look for WARNING/ERROR/CRITICAL log messages above for more details."? And similarly below.

By moving the exception raise down to the outer click layer (returning
None through all higher layers) we avoid the long traceback that hides
the useful information being logged by pipe_base.
The standard library type annotations now indicate that
multiprocessing.get_context returns a not-very-useful object unless
the 'method' argument is exactly one of three string literals, so we
need to annotate accordingly to avoid mypy complaints about that
fallback object.
@TallJimbo TallJimbo merged commit 6e0b8ed into main Sep 30, 2022
@TallJimbo TallJimbo deleted the tickets/DM-36360 branch September 30, 2022 15:01
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

2 participants