Repository navigation
Conversation
…n queries - routing --estimate-only prints the calls, tokens and list-price USD that --model would make, as JSON, and makes no model call. - all-vs-all accepts --repeats with the same noise block as routing, and prints a cost block; the JSON gets the cost too. - all-vs-all refuses queries for a skill that only sets disable-model-invocation, names it, and suggests --include-disabled. - --queries-from-headings falls back to the description's sentences when the headings give no query; queries_source records which one was used. - --seed help says it fixes the card order only and points at --repeats for answer noise. - Routing console lines and query loading move to commands/novelty_routing_report.py to keep novelty.py under its ratchet. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…fuses --estimate-only, repeats count calls The model estimate skipped contentless queries that the run still sends, --all-vs-all --estimate-only made paid calls, the stderr call count left out the repeats, and the model arm's unscored and tied calls were printed as queries. Fix each and cover it with a test. Routing console output moves to commands/novelty_routing_report.py to keep commands/novelty.py under its growth cap. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…ves nothing With neither a heading nor a description sentence, the run refuses with no_queries, but the warning still said the queries were derived from the artifact. Print it only when a query was derived. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…arm repeats With --repeats above 1 the model arm's sibling count is per call, so the attractor line now names its unit the way the tie and unscored line does. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
github-actions
Bot
added
the
cli-interface-change
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): ~ `dokimi novelty routing` parameters changed:
+ added option `--estimate-only`Updated on every push; the |
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 11 selected item(s). Review details
|
| priced = list_cost_usd(args.model, args.provider, estimate["input_tokens"], | ||
| estimate["output_tokens"]) |
There was a problem hiding this comment.
Must-fix: these are run-wide token totals, but list_cost_usd prices them as one completion. For models with context-length pricing tiers (for example Gemini 2.5 Pro), many short calls can therefore cross the long-context threshold only in the estimate and be charged the higher rate, although every real request stays below it. A short query with a sufficiently large --repeats reproduces this; changing repeats should not change the per-call tariff. Price each query's prompt and one pick separately, then sum and multiply by repeats. Add a tiered-pricing test; the current linear pricing mock cannot detect this.
There was a problem hiding this comment.
Fixed in 1c5348b. The estimate now prices each query's prompt and one pick on their own, sums them, and multiplies by the repeats, so a long-context tier applies to one call's prompt as it does in a real run. Pinned by test_estimate_prices_each_call_so_a_long_context_tier_is_not_reached_by_the_run_total, which uses a tiered stub; it failed on the previous head (0.568 against the 0.284 expected).
| f"{len(queries_by_skill)} skills with queries") | ||
| _print_attractors(result) | ||
| print_attractors(result) | ||
| print_all_vs_all_model(model_arm, payload.get("cost")) |
There was a problem hiding this comment.
Must-fix: this reporting call is unreachable whenever any model call fails, because the earlier if failed raises SystemExit. For example, the existing partial-failure test (one successful call followed by two failures), extended with --repeats 3, saves cost/noise in the file but prints neither on stdout. This hides usage from successful calls and breaks the new console-reporting promise. Move this call before the failure exit, while keeping the success-only summary after it, and extend the partial-failure test to assert cost/noise output and a non-zero exit.
There was a problem hiding this comment.
Fixed in 2ccd64b. The cost and noise lines now print before the failure exit, so a partial run shows the usage of the calls that ran. Pinned by test_a_partial_failure_still_prints_the_cost_of_the_calls_that_ran, which found no cost line before the fix. On a successful run those two lines now come before the OK Saved line instead of after the attractors.
There was a problem hiding this comment.
Correction to my earlier reply on this thread. I wrote that the success path was unchanged. It was not: a6fc22d dropped the noise and cost lines from a clean all-vs-all run.
Fixed in 8611ba8. A clean run now prints the summary, the attractors, then the noise and cost lines, as on main. A partial run prints the noise and cost lines before the failure exit, so the usage of the calls that ran is still shown.
Pinned by test_a_partial_failure_still_prints_the_cost_of_the_calls_that_ran and test_a_clean_all_vs_all_run_still_prints_its_cost_after_the_summary.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #329 +/- ##
=======================================
Coverage 94.05% 94.05%
=======================================
Files 297 298 +1
Lines 38018 38104 +86
Branches 6424 6437 +13
=======================================
+ Hits 35756 35840 +84
- Misses 1452 1454 +2
Partials 810 810
🚀 New features to boost your workflow:
|
The estimate priced the run's summed prompt and picks as one completion. A long-context tier applies to one call's prompt, so many short calls could be charged the higher rate although no real request reaches it. Each query's prompt and one pick are now priced on their own, summed and multiplied by the repeats. Thread: PRRT_kwDOVBg7g86yTJiq Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The failure exit came before the cost and noise lines, so a run with some failed calls hid the usage of the calls that succeeded. Those lines now print before the exit; the success summary stays after it. Thread: PRRT_kwDOVBg7g86yTJix Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The per-call pricing added in 1c5348b returns None for an unpriced model; no test reached that path, which left a patch-coverage gap. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…he partial exit CI on a6fc22d failed the growth ratchet: novelty.py was 1149 lines against a baseline of 1147. The partial-failure exit moves into exit_partial_all_vs_all in novelty_routing_report.py, so the command body shrinks. The success path keeps its original order: the summary, then the attractors, then the noise and cost lines. A test pins that the cost line still follows the saved-file line on a clean run. The baseline drops to 1146 as check_growth.py --update reports. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 8611ba8. --estimate-only, --all-vs-all --repeats and the all-vs-all cost block work as described. The description fallback of --queries-from-headings needs another look: its queries are the gold card's own text, so a skill that used to give no_queries now gets a rate near 1.0.
What I ran, at 8611ba8:
make ci-local --offline: 11,990 passed, 0 failed, 58 skipped; all checks pass.- A repro of the description fallback: one-word headings, a three-sentence description, and three competitors that share its words. Lexical R@1 1.0, MRR 1.0,
queries_source: description. --estimate-onlyon an artifact the real run refuses: exit 0,n_calls: 0,expected_usd: 0.0.- An all-vs-all query file that names a disabled catalog skill: the refusal is right, but the field is built twice, so stderr shows two different
--catalogcounts.
Checked, no issues
- Estimate pricing: each call is priced on its own prompt and then multiplied by the repeats, so a long-context tier is not reached by the run total. The tiered-stub test pins this.
- Partial all-vs-all exit: the noise and cost lines print before the failure exit. A clean run prints them after the attractors, as on main.
- Repeats in all-vs-all:
model_routingreturns each query's repeats next to each other, so the slices in_model_armgroup them correctly for_noise. The call count on stderr is multiplied by the repeats. - CLI snapshot and docs:
cli-interface.json, the--seedhelp andskill-novelty.mdmatch the new flags.
| def description_as_queries(description: str, *, limit: int = 40) -> list[str]: | ||
| """Sentences of a description, for triage when the artifact has no usable heading. | ||
|
|
||
| A skill whose body is a ``#`` title and no ``##`` heading of two or more words still says | ||
| what it is for in its description. Each sentence with two or more content words is | ||
| offered, as :func:`headings_as_queries` offers headings. The same self-derived stamp applies. | ||
| """ | ||
| sentences = _SENTENCE_END.split(" ".join(description.split())) | ||
| return [s for s in sentences if len(tokenize(s)) >= 2][:limit] | ||
|
|
||
|
|
||
| def self_derived_queries(body: str, description: str) -> tuple[list[str], str]: | ||
| """The triage queries of an artifact and where they came from. | ||
|
|
||
| Headings are preferred; the description's sentences are the fallback. Returns | ||
| ``(queries, source)`` with ``source`` ``"headings"``, ``"description"``, or ``"none"`` when | ||
| neither gives a query of two or more content words. | ||
| """ | ||
| headings = headings_as_queries(body) | ||
| if headings: | ||
| return headings, "headings" | ||
| sentences = description_as_queries(description) | ||
| return sentences, "description" if sentences else "none" |
There was a problem hiding this comment.
Blocking: The description fallback routes the gold card's own text, so its rates are near 1.0 by construction.
The gold card both arms rank is name + description, and these queries are sentences of that same description, so every query appears word for word in the gold card. Headings at least come from the body, which is not on the card. A repro: gold grill with the description "Grill the user about a plan. Use when stress-testing a design. Ask hard questions about tradeoffs." and only a one-word ## Steps heading, against three competitors that share plan, design, user and questions: queries_source: description, lexical R@1 1.0, MRR 1.0. Before this PR the same skill gave status: no_queries. So the 24 of 144 skills that were "not measured" now get a near-perfect figure, with the same triage stamp as a heading run, and a corpus-wide figure like the 0.225 in the docs moves up because of them. The changelog does not say this.
Suggestion: keep the no_queries refusal for a skill with no usable heading. If the fallback stays, the rate lines and the docs should say that a description-derived rate is a self-match and not comparable with a heading-derived one, and a corpus summary should leave these skills out.
| if args.estimate_only: | ||
| print_routing_estimate(args, candidates, queries) | ||
| return |
There was a problem hiding this comment.
--estimate-only returns before the no-usable-queries refusal, so it prices a run that would be refused as a free, valid run.
Repro: an artifact with the description Stop. and only ## Steps, run as novelty routing --queries-from-headings --model claude-sonnet-4-5 --estimate-only, prints n_calls: 0, expected_usd: 0.0 and exits 0. The same command without the flag exits non-zero with "no usable queries" and writes a no_queries record. The same happens with a --queries file of only stop words. A batch script that budgets with the estimate sees a zero-cost run instead of a refusal.
Suggestion: check tokenize on the queries (as selection_prompt_tokens already does) before the estimate branch, and exit with the same reason the real run gives.
| everyone = _routing_field(argparse.Namespace(**{**vars(args), "include_disabled": True}), | ||
| set()) |
There was a problem hiding this comment.
_refuse_queries_naming_disabled builds the field a second time, so every field warning prints twice, with a wrong competitor count.
Repro: a catalog with one disable-model-invocation skill and a queries file that names it. stderr shows --catalog ...: 5 competitors and then --catalog ...: 6 competitors before the refusal; the second count includes the disabled skill, which is not in the field. Skipped-competitor and no-description warnings repeat too, and a large catalog is read twice.
Suggestion: the first _routing_field call already appends <name>: disable-model-invocation to left_out (the catalog notes too); pass left_out in and take the disabled names from it instead of loading the field again.
| gold=index, seed=seed + index) | ||
| gold=index, seed=seed + index, repeats=repeats) | ||
| rows = routed["per_query"] | ||
| groups += [rows[start:start + repeats] for start in range(0, len(rows), repeats)] |
There was a problem hiding this comment.
The all-vs-all noise.per_query rows pool every skill's queries but do not say which gold each row belongs to.
In a single-artifact run there is one gold, so query is enough. Here two skills can list the same request (profile my binary for perf and for profiler), and you get two rows with the same query and different n_correct, with nothing to tell them apart or to read agreement per skill.
Suggestion: add the gold skill name to each group's row (for example gold) when building groups in _model_arm.
| return [h for h in headings if len(tokenize(h)) >= 2][:limit] | ||
|
|
||
|
|
||
| _SENTENCE_END = re.compile(r"(?<=[.!?])\s+") |
There was a problem hiding this comment.
Nit: The sentence split breaks after any period and space, so e.g. / i.e. / vs. cut one sentence into two queries.
"Profile code, e.g. PyTorch training loops, and report hotspots." gives "Profile code, e.g." and "PyTorch training loops, and report hotspots."; both have two content words, so both are routed.
Suggestion: do not split after a short lowercase token ending in a period, or skip common abbreviations.
|
|
||
| from .client import LLMClient | ||
| from .routing import BM25, Candidate, RoutingInputError, model_routing, tokenize | ||
| from .routing import BM25, Candidate, RoutingInputError, _noise, model_routing, tokenize |
There was a problem hiding this comment.
Nit: all_vs_all.py now imports the private _noise from routing.py.
Someone changing a _ helper in routing.py will assume it has no users outside the module.
Suggestion: rename it to a public name (for example repeat_noise) now that two modules use it.
Summary
A rescan of public skill catalogs (10-30) found five gaps in
novelty routing. This PR closes them:Self-derived queries stay labelled as triage (
triage: self-derived,queries_self_derived: true), andqueries_sourceisheadings,description, ornone. When neither gives a query the run still refuses withno_queries, and the warning no longer claims a derivation that did not happen.Routing console lines and query loading move to
commands/novelty_routing_report.py, socommands/novelty.pyshrinks from 1171 to 1147 lines. The growth baseline is lowered, not raised.Evidence
Red on intel/main: nine tests fail, six in
tests/test_novelty_routing_cli_gaps.py, two intests/test_novelty_routing_all_vs_all.py, andtests/test_novelty_routing.py::test_headings_inside_code_only_refuse_the_run_and_say_why. The two tests added after the first commit (no-derivation warning, attractor unit) were each shown red on the commit before their fix.Local checks on this branch (
aa9fcbeb):Self-review (pr-review local mode, 3 medium passes, diff and description only). Pass 2 found one blocker (the attractor line counted calls under the word "queries"); fixed in
aa9fcbeb, and pass 3 found none. Lenses and verify were not run.Declined review findings
--estimate-onlyexits 0 withn_calls 0where the real run exits 2 withno_queries(no usable query). The estimate prices calls and refuses nothing; a refusal would be a separate change._SENTENCE_ENDsplits after abbreviations such as "e.g.", so a self-derived query can be a fragment. Only the triage source is affected, and it is stamped as triage. Not measured.[a-z0-9]+) splits non-English words and gives CJK no tokens. It predates this PR, andheadings_as_querieshas the same limit. The new description fallback inherits it; a CJK-only description is refused asnone.--estimate-onlyis the spelling the rescan brief asked for.dry_runis the registry concept (dokimi/cli_flags.py);tests/data/cli_flags_allowlist.txtalready admits this spelling fornovelty analyze. Reviewers may prefer--dry-run; that is a rename, not a fix.Not in this PR
description_too_long,name_not_dirandtitle_case_nameare flipped by fix(catalog): flag name conflicts, fail an empty root, keep spec errors out of routing #328, not by this PR.Merge Danger
Door: two-way. The new flag and keys are additive (
queries_source,cost,noise,estimate_only). Reverting restores the old refusals.Blast Radius: low. Routing only; no catalog loading or field building is touched.
Ramifications:
--all-vs-allnow accepts--repeatsand makesNtimes the calls. Scripts that relied on the refusal will see model calls.--queries-from-headingsruns on skills it used to refuse; those runs are triage, stamped as such.docs/reference/cli-interface.jsonis regenerated for the new option.🤖 Generated with Claude Code