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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 19, 2024
Merged

Report notebook warnings #642

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a17970
Report notebook warnings
frankharkins Jan 18, 2024
5cc7f67
De-mystify escape codes
frankharkins Jan 19, 2024
f9b10b7
Fail on warnings
frankharkins Jan 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 46 additions & 3 deletions scripts/nb-tester/test-notebook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

import argparse
import sys
import warnings
import textwrap
from dataclasses import dataclass
from datetime import datetime
from pathlib import Path
Expand Down Expand Up @@ -39,6 +41,39 @@ class ExecuteOptions:
submit_jobs: bool


@dataclass(frozen=True)
class NotebookWarning:
cell_index: int
msg: str

def report(self):
message = f"Warning detected in cell {self.cell_index}:\n"
for line in self.msg.splitlines():
message += (
textwrap.fill(
line, width=77, initial_indent=" │ ", subsequent_indent=" │ "
)
+ "\n"
)
print(f"\033[0;33m{message}\033[0m", flush=True)
frankharkins marked this conversation as resolved.
Show resolved Hide resolved


def extract_warnings(notebook: nbformat.NotebookNode) -> list[NotebookWarning]:
"""
Detect warning messages in cell outputs
"""
notebook_warnings = []
for cell_index, cell in enumerate(notebook.cells):
if not hasattr(cell, "outputs"):
continue
for output in cell.outputs:
if hasattr(output, "name") and output.name == "stderr":
notebook_warnings.append(
NotebookWarning(cell_index=cell_index, msg=output.text)
)
return notebook_warnings


def execute_notebook(path: Path, options: ExecuteOptions) -> bool:
"""
Wrapper function for `_execute_notebook` to print status
Expand All @@ -49,16 +84,23 @@ def execute_notebook(path: Path, options: ExecuteOptions) -> bool:
nbclient.exceptions.CellTimeoutError,
)
try:
_execute_notebook(path, options)
nb = _execute_notebook(path, options)
except possible_exceptions as err:
print("\r❌\n")
print(err)
return False

notebook_warnings = extract_warnings(nb)
if notebook_warnings:
print("\r⚠️")
[w.report() for w in notebook_warnings]
return True
Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Collaborator

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.


print("\r✅")
return True


def _execute_notebook(filepath: Path, options: ExecuteOptions) -> None:
def _execute_notebook(filepath: Path, options: ExecuteOptions) -> nbformat.NotebookNode:
"""
Use nbconvert to execute notebook
"""
Expand All @@ -73,12 +115,13 @@ def _execute_notebook(filepath: Path, options: ExecuteOptions) -> None:
processor.preprocess(nb)

if not options.write:
return
return nb

for cell in nb.cells:
# Remove execution metadata to avoid noisy diffs.
cell.metadata.pop("execution", None)
nbformat.write(nb, filepath)
return nb


def find_notebooks(*, submit_jobs: bool = False) -> list[Path]:
Expand Down