Repository navigation
Conversation
A model-backed `novelty routing` run wrote no record of what it spent, so a routing rate could not be read against its cost. The run now writes a `cost` block into routing.json (calls, input/output tokens, cache read/write tokens, and usd, null when any call is unpriced) and prints the same totals on stdout. Cache token totals are added to the per-role cost summary, so novelty analyze reports them too. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Declare cache_read_tokens and cache_write_tokens (integer, minimum 0) in the novelty.v1 cost block, which routing and analyze both write. Say in skill-novelty.md that cache tokens are reported separately and may or may not be inside input_tokens, depending on the provider. The routing test stub now sets cache_write_tokens too, and asserts its total. The stdout cost line drops the trailing model tag, which the model line above already carries. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
==========================================
+ Coverage 91.98% 91.99% +0.01%
==========================================
Files 223 223
Lines 27743 27754 +11
Branches 4560 4561 +1
==========================================
+ Hits 25520 25533 +13
+ Misses 1574 1572 -2
Partials 649 649
🚀 New features to boost your workflow:
|
antonwolfy
left a comment
There was a problem hiding this comment.
The cost block is right when every call returns a priced usage record, and the JSON totals are correct. Three things are worth fixing before merge: the stdout line when calls are missing or failed, the replayed-call accounting, and the stdout assertions.
What I ran, on a Linux review host at 14a7e93:
make ci-local: 20 passed, 8 failed, 7 skipped. 6883 tests passed, 21 skipped; changed-line coverage, schema-compat vs the base, wheel build and smoke all pass. The 8 failures are ruff, mypy, mkdocs, pip-audit and the other tool steps, missing from my venv becausemake devstops at the tools install on this base, which #179 fixed onmain. Static checks, mypy and the strict docs build pass on GitHub's CI at this head.- Reproductions for the code findings, with the provider call stubbed.
- Mutation run on the new code and tests: mutants of
client.py, the JSON totals and thecostblock are all caught. Four mutants of the printed line pass the suite (inline comment).
Checked, no issues
- Schema: the two new fields are additive optional integers matching their neighbours, and the
costobject has noadditionalProperties: false, so no version bump is needed.routing.jsonitself is not validated by any schema. - Other callers:
roll_up_costhas one other caller (novelty/analyze.py); the markdown report prints only calls, tokens and usd, so it does not change. - Lexical runs: no
costblock is written and the print sits under the sameif args.modelguard.
… cache replays Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…commands/novelty.py for W1 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…baseline after the extraction Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
roll_up_cost sums the new cache_read/cache_write/replayed_calls fields over every role, but only single-client tests covered them, so a total taken from the first role alone (what novelty analyze would then report) passed. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
antonwolfy
left a comment
There was a problem hiding this comment.
Review at d2fd404. The three earlier threads are fixed: the zero-call line, the replayed-call accounting and the full-line assertion all hold. Two things are worth fixing before merge: a change to novelty analyze's existing cost keys that the PR calls additive, and two test gaps found by mutation.
What I ran, on a Linux review host at d2fd404:
make ci-local: 28 passed, 1 failed, 7 skipped. 7213 tests passed, 1 skipped; changed-line coverage, schema-compat vs the base, mypy, ruff, wheel build and smoke all pass. The one failure is pip-audit, which reports a gitpython 3.2.0 advisory (fix 3.2.1) from the lock files; the PR does not touch them.- Reproductions with the LLM transport injected, comparing
roll_up_costat the base and at the head. - Mutation run on the new code against
test_novelty_routing.pyandtest_novelty_measure.py: 23 mutants, 4 of which pass the suite (two inline comments).
This PR changes existing novelty analyze cost values, not only adds keys. analyze.py:386 calls roll_up_cost, and cost_summary (client.py:171-178) now skips replayed rows and sets usd to null when a call returned no usage. Two calls, each 100 in / 10 out / $0.01, replayed=True:
- base:
{'llm_calls': 2, 'input_tokens': 200, 'output_tokens': 20, 'usd': 0.02} - head:
{'llm_calls': 2, 'input_tokens': 0, 'output_tokens': 0, ..., 'replayed_calls': 2, 'usd': 0}
One live call and one call withusage=None: baseusd0.01, headusdNone. So a resumedanalyzerun withDOKIMI_RESPONSE_CACHEnow reportsllm_callsthat no longer matches its token andusdtotals, and the Markdown report (novelty/report.py:468-474, outside this diff) prints "2 LLM calls, 0 in / 0 out, usd 0" without mentioning replays. The new rules are the right ones, and they match the spend cap. But the PR description says "no existing key changes meaning", the changelog fragment files this under Added and namesanalyzeonly for the cache keys, and the docs paragraph sits in the routing section. Suggestion: say in the changelog fragment (as Changed) and in thecostschema descriptions ofllm_calls,input_tokens,output_tokensandusdthat replayed calls are excluded from the totals, printreplayed_callsin thereport.pycost line when it is non-zero, and correct the PR description. Related: with an unpriced replayed row and a priced live row,usdis 0.01, while the docs and schema sayusdisnullwhen any call had no known price. Say "any call this run paid for".
|
|
||
| monkeypatch.setattr(llm, "llm_call_with_usage", failing) | ||
| payload = _model_run(tmp_path) | ||
| assert payload["cost"]["llm_calls"] == 0 |
There was a problem hiding this comment.
Nothing pins usd for a client that made no calls. This test asserts llm_calls == 0 and the printed text, but not payload["cost"]["usd"].
Mutation check: removing self.calls > 0 from priced (client.py:178) makes an idle client report usd 0.0, which reads as a free run. The full routing and measure files still pass (71 passed), because the 0 calls line hides the value.
Suggestion: add assert payload["cost"]["usd"] is None after line 545.
There was a problem hiding this comment.
Pinned in 2594a80: test_a_model_run_whose_every_call_fails_prints_no_price_blame now asserts payload["cost"]["usd"] is None (fails with self.calls > 0 dropped).
| from dokimi.metrics.usage import UsageRecord | ||
|
|
||
| _stub_provider(monkeypatch, UsageRecord(prompt_tokens=100, completion_tokens=5, | ||
| cost_usd=0.002, replayed=True)) |
There was a problem hiding this comment.
The replay test never gives the replayed rows cache tokens, so cache totals on replayed rows are unpinned. The UsageRecord here has no cache_read_tokens or cache_write_tokens, and the test asserts neither.
Mutation check: summing cache_read and cache_write over self.usage instead of billed (client.py:174-175) passes both files (71 passed), so replayed cache tokens would be billed as this run's.
Suggestion: build the record with cache_read_tokens=60, cache_write_tokens=30 and assert cost["cache_read_tokens"] == 0 and cost["cache_write_tokens"] == 0. Also assert "replayed" not in out in test_a_model_run_records_its_calls_tokens_and_cost: a mutant that always prints the replayed suffix also passes today.
There was a problem hiding this comment.
Fixed in 2594a80: test_replayed_calls_are_counted_apart_and_not_billed now gives the replayed rows cache_read 60 / cache_write 30 and asserts both totals are 0 (fails when summed over self.usage); test_a_model_run_records_its_calls_tokens_and_cost asserts "replayed" not in out.
|
antonwolfy
left a comment
There was a problem hiding this comment.
Review at a5b007e. The only change since my last review is a test that pins the cross-role roll_up_cost sums, and it holds. The analyze behaviour change and both test gaps from d2fd404 are still open, plus one more wording point on usd.
What I ran, on a Linux review host at a5b007e:
make ci-local: 28 passed, 1 failed, 7 skipped. 7214 tests passed, 1 skipped; ruff, mypy, schema-compat vs the base, docs build, wheel build and smoke all pass. The one failure ispip-audit, which reports an advisory from the lock files; this PR does not touch them.- Mutation run on
cost_summary,roll_up_costand the printed routing summary againsttest_novelty_routing.pyandtest_novelty_measure.py: 33 mutants, 8 of which pass both files. Three of them are the ones in my open threads; the rest are print-only lines of_print_routing_summarythat predate the PR's logic. - Reproductions with the LLM transport injected.
Still open from my earlier review:
dokimi/tests/test_novelty_routing.py:545: nothing pinsusd is Nonefor a client with no calls; droppingself.calls > 0atclient.py:178still passes.dokimi/tests/test_novelty_routing.py:569: the replay test gives the replayed rows no cache tokens; summing cache tokens overself.usageinstead ofbilled(client.py:174-175) still passes, and so does printing the replayed suffix unconditionally.
-
This PR still changes existing
novelty analyzecost values, not only adds keys.analyze.pycallsroll_up_cost, which now takescost_summary's rules: replayed rows are left out of tokens andusd, andusdisnullwhen a call returned no usage. Two replayed calls, each 100 in / 10 out / $0.01, giveinput_tokens: 0, output_tokens: 0, usd: 0, replayed_calls: 2at the head, against 200 / 20 / 0.02 at the base.llm_callsstill counts them (2), so calls and tokens describe different sets. Nothing else moved with it:novelty/report.py:468-473(outside this diff) prints only calls, tokens andusd, so a cachedanalyzererun reads "2 LLM calls, 0 in / 0 out, usd 0" with no mention of replays, andusd Nonefor a call with no usage.docs/benchmarking-and-comparison.md:691-693(outside this diff) still says a replayed entry reports the stored usage "so cost stays comparable across runs".- The changelog fragment files it all under Added; the
usd,llm_calls,input_tokensandoutput_tokensdescriptions inschemas/novelty.v1.jsondo not mention the exclusion.
Suggestion: move theanalyzepart of the fragment to Changed, add the exclusion to those schema descriptions, printreplayed_callsin thereport.pycost line when it is non-zero, fix the benchmarking doc, and correct the PR description ("no existing key changes meaning").
-
usdis notnull"when any call had no known price" if the unpriced call was replayed. The check runs over non-replayed rows only (client.py:178). One live call (5 in, $0.1) plus one replayed call with no price givesllm_calls: 2, input_tokens: 5, replayed_calls: 1, usd: 0.1. A single unpriced replayed call givesusd: 0and the routing line prints$0.0000. The changelog,docs/skill-novelty.md:611and thecost_summarydocstring all say "any call". Excluding replayed rows is the right rule; the wording is not. Suggestion: say "any call this run paid for" in the fragment, the doc paragraph, the docstring and the schemausddescription.
@xaleryb's question on whether analyze keeps stored totals or this-run spend, and whether zero calls give usd None or 0.0, waits for the author.
Checked, no issues
- Schema: the new
costfields are optional integers added in place, so no version bump orSCHEMA_VERSIONSchange is needed. roll_up_cost: zeroing any one of the calls, input, output, cache read, cache write or replayed sums fails the suite, and the new cross-role test covers the first-role-only total.- Routing line: the zero-calls branch, the unknown-USD text, the
.4fformat and swaps of read/write or input/output are all caught.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 54f4ca8, which fix the nit from my review:
54f4ca8:dokimi/tests/test_novelty_measure.py:247: added the second blank line beforetest_a_failed_call_is_a_dropped_pair_not_a_graded_answer.
make ci-local at 54f4ca8: 28 passed, 1 failed (pip-audit, as at the reviewed head), 7 skipped.
Not approving yet. Still open: the two risks from my review; the two earlier threads of mine above nit level.
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/commands/novelty.py # dokimi/scripts/growth_baseline.json # dokimi/tests/test_novelty_routing.py
…ay 'this run paid for' Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Review-body items at 2594a80 (after merging main in 21a1e0d, which had conflicts in novelty.py, growth_baseline.json and test_novelty_routing.py):
|
Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/scripts/growth_baseline.json
Part of #4.
Summary
A
novelty routing --modelrun now records what it spent.The cache token counts are also added to the per-role summary that
novelty analyzealready prints, so itscostblock reports them too. A run without--modelmakes no calls and writes nocostblock.Evidence
Three new tests in
tests/test_novelty_routing.py. The two model-run tests fail with theintel/mainversion ofcommands/novelty.pyandnovelty/client.py(nocostblock, no cache fields) and pass on this branch:Local checks on the rebased head:
Merge Danger
Door: two-way. Every change is additive: new keys in the routing JSON and in the analyze cost summary, and no existing key changes meaning. Reverting restores the old output.
Blast Radius: low, limited to
--modelrouting runs and the analyze cost summary.schemas/novelty.v1.jsongainscache_read_tokensandcache_write_tokensin the cost summary. Readers that validate strictly will see two new optional-looking integer fields.usdisnull, not0, when any call had no price, so a missing price is not read as a free run.input_tokensdepends on the provider. The docs say so; the two counts are reported separately.Checklist
pytestpassesruff,mypy, and schema checks passdocs/skill-novelty.md(routing cost block)🤖 Generated with Claude Code