[Refactor] Decouple config and execution in nb-tester #2410
Conversation
|
|
||
| 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.
There was a problem hiding this comment.
Good spot! I plan to move that functionality to our GitHub action because it's very specific to our repo, but it shouldn't have been removed in this PR.
There was a problem hiding this comment.
It used to live there, but we decided to move it to the script in #2062. I'm fine with moving this back to the GH action, so long as it preserves the same prior functionality of not failing when the notebooks couldn't have been tested anyways.
There was a problem hiding this comment.
I remember that, here's what I had in mind. It is the kind of thing I'd forget so thanks for reminding me.
documentation/.github/workflows/notebook-test.yml
Lines 71 to 88 in 9dbd215
I've moved this to config to keep the args hidden from the rest of the program.
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.