-
Notifications
You must be signed in to change notification settings - Fork 85
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
Report notebook warnings #642
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
scripts/nb-tester/test-notebook.py
Outdated
if notebook_warnings: | ||
print("\r⚠️") | ||
[w.report() for w in notebook_warnings] | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this return False
? We want to error if there were warnings imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set it as True
because some warnings are out of our control, e.g. if a Qiskit function uses a deprecated numpy function.
It depends if you'd rather have false positives or false negatives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we require you to allow-list deprecations? I'm concerned if we don't error, then no one will ever notice the warnings. People don't check CI logs proactively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, maybe I'm over-planning; I'll set it to False
and we can revisit it if we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because some warnings are out of our control, e.g. if a Qiskit function uses a deprecated numpy function.
Ideally that will prompt us to go fix Qiskit itself. In the meantime to them fixing it, we could allow-list it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The notebook testing script does not report any warnings raised in notebooks. It's important to report these so we can fix deprecations before they're removed from Qiskit, and to spot any unnecessary warnings that might harm the user experience. I've tried this in the past and the best method is to search the executed notebook outputs for warnings. That's what I do here. ### Screenshots <img width="698" alt="Screenshot 2024-01-18 at 12 45 19" src="https://github.com/Qiskit/documentation/assets/36071638/39438185-5f70-4df3-b3e4-4f483481cd65">
The notebook testing script does not report any warnings raised in notebooks. It's important to report these so we can fix deprecations before they're removed from Qiskit, and to spot any unnecessary warnings that might harm the user experience.
I've tried this in the past and the best method is to search the executed notebook outputs for warnings. That's what I do here.
Screenshots