Repository navigation
feat: add --explain to show the planned requests before they are made - #84
Conversation
Implements ondata#74 as a dry run: the plan goes to stderr, stdout stays empty, and the run stops before the first request, so nothing is spent. - The hook sits in `sdmx_request`, the shared path every SDMX call funnels through, plus the StatKit hub's own httpx client in `hub.py`, which bypasses it. Plan lines show method, URL (httpx's encoder, query string included) and the POST body. - Not fetching means the planned URLs have to be *known*: cache hits report the request they replaced (catalog, datastructure, codelist, categoryscheme and categorisation). For hub-only providers those URLs are built elsewhere, so no line is printed rather than a wrong one. - `ExplainStop` derives from `BaseException` so the callers' `except Exception` handlers cannot report the plan's stop as an error; `main()` exits 0 and says "no provider request would be made" when the whole plan came from the cache. - Applies to the nine network commands; `search --semantic` is a no-op, since Ollama is not a provider request. - Nothing is written in a dry run: the console, `_emit`, `_write_output`, the chart and the query file are all guarded. Verified: 418 tests (14 new), ruff clean, mypy strict clean on 16 files.
|
[Medium risk] Adds a dry-run mode to preview requests without executing them. The PR appears safe to merge; the catalog-outage finding is fixed, and no new actionable issues were found. SummaryAdds
Reviews (3) · Last reviewed commit: "cli: on a siblings catalog outage, skip ..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Reproduced, thanks — it is real. With a warm I am deliberately not making cache reads read-only in this PR (the PR body now says so instead of claiming "nothing is written"):
The useful half of your suggestion — never create a missing database during a plan — is a clean follow-up; I would keep the migration and skip only the creation. Say the word and I will do it as a separate PR. |
aborruso
left a comment
aborruso
left a comment
There was a problem hiding this comment.
@paoValle thanks. I checked 734abcd: 418 passed, ruff and mypy clean. I ran every dry run with the network pointed at a dead proxy, so no request could leave. With a cold cache (ISTAT, Eurostat, INPS, OECD): exit 0, empty stdout, no file written. With a copy of my warm cache the [cached] lines are right and the cache directory is byte-identical before and after; with the structure cached, get shows the full data URL.
Two points inline. On cache.db being initialised by a read, the reasoning in the PR body is fine for me.
The plan named the same request twice or read like a real download: - `get` without --last-n/--first-n/--yes stops at the size probe, whose line was byte-identical to the request of a real `--last-n 1` run. The funnel now takes a probe marker and prints `[would fetch] (size probe) GET ...`, with the exact URL the non-probe call builds (format param included). - `siblings` read the dataflow catalog once for id resolution and once for the description table, so a warm-cache plan listed the same `[cached]` dataflow line twice. The CLI now loads the catalog once and passes it to both callers; the fallback paths in resolution and in `siblings_of` keep their old behaviour when the catalog is unreachable. Tests: the get plan asserts the (size probe) marker and the absence of a plain probe line; a new test pins one catalog line in the siblings plan. Both fail without their fix (mutation-checked). 419 passed.
|
Both points fixed in The probe marker threads through the request funnel ( And Tests: the |
…descriptions Greptile: when the first all_available() fails, the previous fallback (dataflows=None) made both resolve_dataflow and siblings_of load the catalog again, so an outage cost three attempts instead of two, each with its own retries. On failure the command now carries on with the raw id and an empty descriptions table, so neither call retries; the category cache still answers. A test pins exactly one load attempt during an outage (fails without the change). 420 passed.
|
Done — And thanks for verifying the probe label and the single catalog line on your warm-cache run. |
2 participants
Closes #74.
I implemented the dry-run variant:
--explainprints the plan to stderr and stops before the first request, so the request is never spent. That is the variant the rate-limit problem needs — a trace still spends the request you are trying to sanity-check — and it covers provenance too, since the plan names the exact URL, method and POST body.What changes
base.sdmx_request()reports the call and stops before the lock and the rate-limit timer.hub._hub_get_json()is hooked as well, because the StatKit hub builds its ownhttpxclient and would otherwise really call it during a dry run. No per-command, per-provider or per-endpoint branching.getcan show the data URL at all —make_url_key()can read the dimension order from the cached structure._emitand_write_outputare guarded),plotwrites no chart (also for a local CSV input, which has no request to plan),get --query-filewrites no YAML, and no provider data is written to the cache. Reading the local cache stays a read that can initialise it, as any command does — see the last note below.ExplainStopderives fromBaseExceptionrather thanException, so the callers' ownexcept Exceptionhandlers cannot report the plan's stop asError: ....main()catches it and exits 0, and saysexplain: no provider request would be made.when the whole plan came from the cache.search --semanticis a no-op under--explain, since the Ollama query is not a provider request.--provider, so it is written after the command —opensdmx get ... --explain, as in the acceptance criteria — notopensdmx --explain get ....Deliberately not done
availableconstraintproduced it. Those steps get no[cached]line rather than a wrong one; a plan with no[would fetch]line and the closingexplain: no provider request would be made.means everything needed was already local.Verification
Cold cache, fresh
OPENSDMX_CACHE_DIR: the plan is printed, stdout is 0 bytes, exit 0, and no cache or rate-limit file is created — i.e. no request was made, including against the rate-limited provider, which is the whole point (ISTAT,rate_limit: 15.0):Warm cache — the per-line cached/network shape from the issue, with the data URL built from the cached structure:
The tests pin "spends nothing" with
httpxpatched andassert_not_called()— the funnel, the hub client and every CLI case - plusstdout == ""in all three output modes and no file written byplot,_write_outputor--query-file. I mutation-checked the guards: removing the console/write guards or the per-invocation reset makes the suite fail.Three things worth knowing:
getwithout--last-n/--first-n/--yesspends a size probe (lastNObservations=1) before the real download; the plan shows that request, so it is finally visible for what it is.cache.db. With a warmdataflows.parquetand nocache.db, one catalogue read creates the empty schema file (45 KB) — the same thing any command does on first use, no provider data in it. Making the cache strictly read-only is not part of this PR:_db_connalso runs the in-placecode_parent/code_ordermigration that older databases need before they can be read, so skipping the writes would either break reading an oldcache.dbor require the readers to tolerate missing columns. Worth a follow-up if the maintainer wants that guarantee; the size probe note below is the same kind of honesty.--explainoffsys.argv, the way the existing--helpcheck does it: the value arrives with the command, and a plan has to be printable while the provider is unreachable. A command line where--explainis a value (--grep --explain) only loses the friendly "API unreachable" hint.