[Refactor] Decouple config and execution in nb-tester #2410
Conversation
| @dataclass | ||
| class Result: | ||
| ok: bool | ||
| reason: str | None = None | ||
|
|
||
| def __bool__(self) -> bool: | ||
| return self.ok |
There was a problem hiding this comment.
Aside from separating the code and introducing the NotebookJob interface, this is the only other change. It's useful to know "why" a bool is set so we can give feedback to users. This was introduced for the write option but I've switched most functions that return a bool to return a Result object.
| return notebook_warnings | ||
|
|
||
|
|
||
| async def execute_notebook(job: NotebookJob) -> Result: |
There was a problem hiding this comment.
This function is logically the same, but it gets its information from the NotebookJob rather than the separate arguments.
|
|
||
| print( | ||
| f"⚠️ Cancelling {len(jobs)} job(s) created after {start_time}.\n" | ||
| "Add any notebooks that submit jobs to `notebooks-that-submit-jobs` in " | ||
| f"`{config_path}`." | ||
| f"`scripts/config/notebook-testing.toml`." |
There was a problem hiding this comment.
Hardcoded for now but will make more general in a follow-up
| if paths and args.is_fork: | ||
| print( | ||
| "⛔️ We can't run notebook tests on pull requests from forks becaus of how GitHub Secrets work." | ||
| "\n\n" | ||
| "If you have write access to Qiskit/documentation, push to a new " | ||
| "branch there and make your pull request from that branch instead." | ||
| "\n\n" | ||
| "If you don't have write access, you should locally test out the notebooks you're modifying " | ||
| "by using the instructions in " | ||
| "https://github.com/Qiskit/documentation#execute-notebooks. " | ||
| "When this PR is approved, a maintainer will merge it to a new " | ||
| "branch in Qiskit/documentation, then make a PR from that branch " | ||
| "into main so it can pass CI.\n", | ||
| ) |
There was a problem hiding this comment.
I wasn't able to find this conditional after the refactor. Do we still use it?
There was a problem hiding this comment.
Good catch. It'd be helpful to keep this.
|
|
||
| for path in config.notebooks_to_execute(): | ||
| backend_patch = None | ||
| if config.should_patch(path): |
There was a problem hiding this comment.
Very nice having this part here rather than in _execute_in_kernel 🚀
|
|
||
|
|
||
|
|
||
| def get_args() -> argparse.Namespace: |
There was a problem hiding this comment.
Maybe we can keep this function in __init__.py to reduce the complexity of the config.py script. Although, I think it's fine either way if you like it more here :)
There was a problem hiding this comment.
I think it makes sense here since it's about config
|
|
||
|
|
||
|
|
||
| def get_args() -> argparse.Namespace: |
There was a problem hiding this comment.
I think it makes sense here since it's about config
| """ | ||
|
|
||
|
|
||
| def post_process_notebook(nb: nbformat.NotebookNode) -> nbformat.NotebookNode: |
There was a problem hiding this comment.
Nit: should this go in execute? It's not configurable. Or even a new version_info.py or post_process.py?
|
|
||
|
|
||
| def cancel_trailing_jobs(start_time: datetime, config_path: str) -> bool: | ||
| def cancel_trailing_jobs(start_time: datetime) -> Result: |
There was a problem hiding this comment.
I'd probably expect this to be in execute.py or a post_process.py so that __init__.py is only the very high-level business logic.
Pre-work for #2405.
This PR refactors nb-tester to decouple the config logic, which decides which notebooks to run and how to patch them, from the notebook execution logic, which runs the notebooks.
The config logic is already the most complicated part of the program. This PR decouples config and execution by moving them to separate files and having them communicate only by a single
NotebookJobinterface. EachNotebookJobcontains a notebook path and instructions on how to run that notebook. This means the config logic can be scoped to creating a list ofNotebookJobobjects. This also makes the config logic easier to test.There should be no behaviour changes in this PR.