feat(scripts): model what each example under examples/ requires - #597
Open
williedoran-neo4j wants to merge 4 commits into
Open
feat(scripts): model what each example under examples/ requires#597williedoran-neo4j wants to merge 4 commits into
williedoran-neo4j wants to merge 4 commits into
Conversation
williedoran-neo4j
force-pushed
the
willie/examples-requirement-model
branch
from
August 11, 2026 10:16
f956372 to
b1bab9e
Compare
This was referenced Aug 11, 2026
williedoran-neo4j
force-pushed
the
willie/examples-requirement-model
branch
3 times, most recently
from
August 12, 2026 10:30
31ef2cc to
e3a12f3
Compare
This was referenced Aug 12, 2026
williedoran-neo4j
force-pushed
the
willie/examples-requirement-model
branch
from
August 12, 2026 12:04
e3a12f3 to
29fafdb
Compare
Nothing knows what an example needs in order to run, so a contributor finds out by running it and reading the traceback. This adds one source of truth the tooling can share. Requirements come from three places. Packages are resolved by an AST import scan that maps *library symbols* to extras - an example imports OpenAILLM, not openai, so the dependency is invisible from its imports alone. Env vars and datastores are read from os.getenv calls and connection URIs in the source. Anything that leaves no trace - APOC, a pre-existing index, outbound internet, or a component named only in a YAML config - is declared per path in SERVICE_RULES. Stdlib-only by design: it has to run before uv sync, which is exactly when somebody most needs to be told what to install. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All three workflows skipped `uv sync` whenever the venv cache key hit exactly. That trusts the cache to be complete, and it is not always: a cache saved from an interrupted install is restored on every later run with the same key, and the install that would repair it is precisely what gets skipped. Seen on neo4j#596, which changes uv.lock and so minted a fresh key: four e2e jobs failed with `ImportError: cannot import name 'WeaviateNeo4jRetriever'` and the same for Pinecone and Qdrant. The restored .venv was missing the vector-store extras, the test step installed only the 82 default packages, and re-running reproduced it exactly - the same cache was restored each time. Deleting the cache entry fixed it. The failure mode is nasty out of proportion to its cause: it presents as a flaky test in code that did not change, it survives re-runs, and it persists until the cache expires or somebody deletes it by hand. Any PR touching uv.lock is first to hit a new key, so it lands on whoever is least likely to suspect CI. uv sync takes a few seconds when the environment is already correct, which is cheap next to that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
williedoran-neo4j
force-pushed
the
willie/examples-requirement-model
branch
from
August 13, 2026 08:37
af0a41d to
d9b6101
Compare
Review feedback on neo4j#597. Two of the tests did not exercise what they claimed, and nothing guarded the tables against a partial edit. - test_an_aliased_symbol_still_maps_to_its_extra never fed the parsed symbols to anything: it built an ExampleRequirements by hand and asserted its extras were empty, which is true by default_factory whatever the code does. The symbol to extra join it names was never executed. - test_install_hints_reports_a_package_behind_no_extra asserted the corpus reports nothing, which is also what the bug it names produced. Both reached for hand-built objects because analyse() raised ValueError on any path outside examples/, so a synthetic file could not be analysed at all. It now returns instead, and both tests run the real pipeline over a temp file. SERVICE_RULES specs are now a TypedDict. They were dict[str, object] and read back through spec.get(), so a mistyped key ("service") or a scalar where a list belongs both silently did nothing - the example quietly lost its requirement. mypy already runs over scripts/ in CI and now rejects both. Adds the invariants that were missing entirely. SYMBOL_EXTRAS and SYMBOL_PROVIDERS are two halves of one fact: their values legitimately differ, since AzureOpenAILLM needs the openai extra but talks to the azure provider, but a symbol in one and not the other means the doctor installs a package it never asks for a key for, or the reverse. Nothing caught that. Nor a service name no label covers, which service_available() reports as satisfied because it cannot probe it. SERVICE_RULES matches are now pinned by count. Asserting only that a pattern matches something protects the nine rules matching exactly one file; the other ten match several, so renaming one of them leaves the pattern non-empty and the requirement silently lost. The count also catches a glob widened until it sweeps in a file that does not want the requirement. Also: read_text() now specifies utf-8, one example being non-ASCII; a duplicate sibling_modules assignment is removed; and install_hints' test is renamed to what it actually asserts, the import name rather than the distribution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the previous commit. Making the install unconditional stops a bad cache entry causing failures, but does not stop one being created. actions/cache saves in a post step, which also runs when the job was cancelled or failed - so a run interrupted during uv sync publishes a half-installed .venv under a key derived from uv.lock alone. Splitting into restore/save makes the save an ordinary step, so an interrupted install never reaches it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
williedoran-neo4j
marked this pull request as ready for review
August 13, 2026 09:13
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.
First of four PRs. The order matches how the tools get used: work out what each example needs (this) → report what is missing (#599) → walk through fixing it (#602) → run what the machine supports (#598).
Nothing knows what an example needs in order to run, so a contributor finds out by running it and reading the traceback. This adds one source of truth the rest of the tooling shares.
Where requirements come from
OpenAILLM, notopenai, so the dependency is invisible from its imports alone. The substring scan this replaces treated any file containing the word "google" as a Vertex example and could not tellOpenAIEmbeddingsfromAzureOpenAIEmbeddings.os.getenvcalls and connection URIs in the source.SERVICE_RULES, because it leaves no trace in the source.Stdlib-only by design: it has to run before
uv sync, which is exactly when somebody most needs to be told what to install.Keeping the tables honest
The tables are hand-maintained, and a half-finished edit produced a plausible wrong answer rather than an error. Each of these is a one-line test:
SYMBOL_EXTRASandSYMBOL_PROVIDERSare two halves of one fact. Their values legitimately differ —AzureOpenAILLMneeds theopenaiextra but talks to theazureprovider — so they cannot be merged, but their keys must not drift. A symbol in one and not the other means the doctor installs a package it never asks for a key for, or the reverse.service_available()returnsTruefor anything it cannot probe, so a typo'd service name would otherwise report as satisfied.SERVICE_RULESmatches are pinned by count. Asserting only that a pattern matches something protects the nine rules matching exactly one file; the other ten match several, so renaming one of them leaves the pattern non-empty and the requirement silently lost. The count also catches a glob widened until it sweeps in a file that does not want the requirement. Update it deliberately whenexamples/gains or loses a file — that is the moment to confirm the rule still applies.SERVICE_RULESspecs are aTypedDictrather thandict[str, object]. Read back throughspec.get(), a mistyped key ("service") or a scalar where a list belongs both silently did nothing. mypy already runs overscripts/in CI and now rejects both.Two CI commits, which do not belong to the model
Called out because they are unrelated to everything above, and would otherwise be a surprise in the diff.
Four e2e jobs failed on #596 with
ImportError: cannot import name 'WeaviateNeo4jRetriever'— and the same for Pinecone and Qdrant — on a commit that changed two Markdown files. No test was flaky: the job restored a cached.venvmissing the vector-store extras and, because installation was skipped on an exact cache hit, nothing repaired it. Re-running reproduced it exactly; deleting the cache entry fixed it.ci: always install dependencies, even on a cache hitstops a bad entry causing failures.ci: never publish a half-installed venv to the cachestops one being created:actions/cachesaves in a post step that also runs when a job is cancelled, so an interrupteduv syncpublishes the partial venv. Splitting intorestore/savemakes the save an ordinary step, which an interrupted install never reaches.They are here rather than in their own PR to avoid opening another one; happy to lift them out if a maintainer would rather they merged independently, since they fix CI for everyone and this stack is four deep.
Scope
Deliberately just the model, its tests and those CI fixes. The model has no consumer until #599 — which is what keeps this small enough to check by reading.
Type of Change
Complexity
Complexity: Low
How Has This Been Tested?
29 tests over the scan, the per-path rules and the table invariants. Each new invariant was confirmed to fail when the thing it guards is broken — dropping a symbol from one table, and simulating a rename against the rule counts — rather than merely passing today.
scripts/sits outside the coverage gate ([tool.coverage.run] source = ["src"]), so these tests are additive and cannot move the 90% number. They need one line of wiring,pythonpath = ["scripts"].Checklist
🤖 Generated with Claude Code