chore: add repo check harness with notebook stripping#119
Open
adnanrhussain wants to merge 2 commits into
Open
chore: add repo check harness with notebook stripping#119adnanrhussain wants to merge 2 commits into
adnanrhussain wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a repo-wide Python “check harness” under scripts/ and wires it into CI to prevent transient Jupyter notebook state (outputs/execution counts/cell metadata) from being committed, along with stripping currently-dirty notebooks.
Changes:
- Introduces
scripts/check.pyand a check registry/contract (scripts/checks/*) with an initialstrip-notebookscheck driven bynbstripout. - Adds a pinned
scripts/requirements.txtandscripts/README.mddocumenting local usage and extension points. - Adds a new CI workflow to run the harness and commits notebook cleanups (state-only stripping).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/requirements.txt | Pins nbstripout used by the harness for consistent local/CI behavior. |
| scripts/README.md | Documents harness setup/usage and planned future checks. |
| scripts/checks/base.py | Defines the check/result/violation contract and git-tracked file discovery helper. |
| scripts/checks/init.py | Registers available checks for the harness. |
| scripts/checks/strip_notebooks.py | Implements notebook stripping/verification via nbstripout. |
| scripts/check.py | CLI entrypoint that runs checks and prints a consolidated report. |
| .github/workflows/checks.yml | Runs the harness in CI on pushes/PRs. |
| evals/vocabulary_evaluator.ipynb | Strips transient cell metadata from the notebook. |
| evals/text_complexity_combo.ipynb | Strips transient metadata/outputs and normalizes execution counts. |
| evals/sentence_structure_evaluator.ipynb | Strips transient metadata/outputs. |
| evals/grade_level_evaluator.ipynb | Strips transient metadata/outputs and clears execution counts. |
| evals/feedback/productive-coaching-writing-feedback/manageable/example_notebook.ipynb | Strips transient cell metadata. |
| evals/feedback/productive-coaching-writing-feedback/appropriate-feedback/example_notebook.ipynb | Strips transient metadata/outputs/execution counts. |
| evals/feedback/productive-coaching-writing-feedback/anchored-in-student-response/example_notebook.ipynb | Strips transient cell metadata. |
| evals/feedback/productive-coaching-writing-feedback/actionable-revision/example_notebook.ipynb | Strips transient metadata/outputs/execution counts. |
| evals/feedback/productive-coaching-writing-feedback/acknowledges-strength/example_notebook.ipynb | Strips transient cell metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from .base import Check, Result, Violation, tracked_files | ||
|
|
||
| # Cell metadata nbstripout does not strip by default but we want gone. | ||
| _EXTRA_KEYS = "cell.metadata.application/vnd.databricks.v1+cell cell.metadata.jupyter" |
There was a problem hiding this comment.
Do we need to consider change management for Databricks here? ie what is v1 referencing? How to manage changes?
Contributor
Author
There was a problem hiding this comment.
While the vendor specific format is intentioned for versioning upstream, I dont think we have activated versioning in the build process yet (no cell in any notebook has a v2). If we do see those keys flow through, we dont need to track them in this final state, and we can then update the filter to just the mime-type to include all versions in the strip-out
georgemelvin
approved these changes
Jun 30, 2026
georgemelvin
left a comment
georgemelvin
left a comment
There was a problem hiding this comment.
Looks good:
- cleans execution metadata from existing repo notebooks
- implements mechanism to clean notebooks locally and in CI
- includes new CI workflow
czi-fsisenda
approved these changes
Jul 1, 2026
czi-fsisenda
left a comment
czi-fsisenda
left a comment
Contributor
There was a problem hiding this comment.
🚀 The constantly changing metadata has been a pain!
| - One check per concern, two modes: **check** (the default — non-zero exit on | ||
| problems) and **`--fix`** (repair in place where safe). | ||
| - The orchestrator runs **every** check and reports **all** failures at once — | ||
| no stop-at-first, no whack-a-mole. |
|
|
||
| ```bash | ||
| python scripts/check.py # check everything (what CI runs) | ||
| python scripts/check.py --fix # auto-fix what's safe, report the rest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a repo-wide check harness under
scripts/, with its first check: stripping transient state from Jupyter notebooks. The same harness runs locally and in CI.Why
Notebook
outputs/execution_count/ cell metadata kept landing in source control (e.g. recent eval PRs), causing noisy diffs, merge conflicts, and a risk of leaking data/keys. There was no automation catching it. This establishes a self-service pattern: break early, with clear info, auto-fix when possible.How it works
--check(verify, non-zero exit) and--fix(repair in place).strip-notebooksdrives pinnednbstripout==0.9.1(scripts/requirements.txt) with--keep-id(no cell-id churn) and--extra-keysfor the Databricks + Jupyter (outputs_hidden) cell metadata nbstripout keeps by default..github/workflows/checks.yml) runs the same harness on PRs/pushes and fails only (never auto-fixes).This PR also
Strips the 9 currently-dirty tracked notebooks. Verified: 0 source-content changes (state-only); the 5 already-clean notebooks are untouched.
Follow-ups (noted in
scripts/README.md)eval-config validation, naming/structure checks, delegation to the SDK suites, and a root-README/CONTRIBUTING pointer once there are more checks.