Repository navigation
feat(routing): estimate-only, all-vs-all repeats and cost - #329
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 |
| 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #329 +/- ##
==========================================
+ Coverage 94.37% 94.41% +0.03%
==========================================
Files 312 314 +2
Lines 40068 40347 +279
Branches 6801 6847 +46
==========================================
+ Hits 37816 38093 +277
- Misses 1436 1438 +2
Partials 816 816
🚀 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.
There was a problem hiding this comment.
Waiting on a decision before I change this: should the description fallback stay?
Context: your point holds. A description-derived query scores R@1 1.0 and MRR 1.0 against its own gold card (a 3-sentence description, three competitors: queries_source: description). Before this PR those skills gave no_queries.
Options:
(a) Keep the no_queries refusal for a skill with no usable heading (your first suggestion). The description queries are dropped, and the "description queries" part of the PR goes.
(b) Keep the fallback, stamped as a self-match: the rate lines and the docs say it is not comparable with a heading rate, and a corpus summary leaves those skills out.
My recommendation: (a). A rate near 1.0 by construction should not reach a corpus figure, and (b) needs changes in the summary code too. The thread stays open until the author picks.
There was a problem hiding this comment.
Blocking: Option (a), please: keep the no_queries refusal for a skill with no usable heading.
One more reason: main already pins that refusal. test_headings_as_queries_on_a_skill_with_no_headings_is_a_usage_refusal from #327 expects exit 2 and a no_queries record for the door-check shape, and 66341f4 flips it to expect a description run with status: ok. With (a) that test goes back to main's version. description_as_queries, self_derived_queries, _sentences and the description part of queries_source, the help text, the docs and the changelog bullet go too.
There was a problem hiding this comment.
Maintainer decision: (a), keep the no_queries refusal; the description fallback is dropped from this PR. Done in 1e2fa05.
The PR title and body are updated to match. A test pins the refusal for a skill with a description and no usable heading.
| 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.
There was a problem hiding this comment.
Fixed in 0fdc281. --estimate-only now refuses with the real run's reason when no query has a content word: exit 2 with no usable queries, nothing estimated, nothing written. Pinned by test_estimate_only_refuses_the_run_the_real_run_refuses (a --queries-from-headings case and a stop-words-only file), which failed before the change.
The PR body still says the estimate exits 0 for such a run (its "declined" bullet). That line is out of date now and needs the author's update.
| 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.
There was a problem hiding this comment.
Fixed in 0fdc281. The field is read once. The loader reports the names it left out for disable-model-invocation (--competitors and --catalog), and the refusal takes them from that list, so each field warning prints once and the count is right. Pinned by test_a_disabled_skill_named_by_a_query_warns_once_and_counts_the_field_once, which failed before the change.
| 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.
There was a problem hiding this comment.
Fixed in 0fdc281. Each row of an all-vs-all noise block now names its gold skill, so two rows with the same query can be told apart. Pinned by test_every_noise_row_names_the_gold_skill_it_belongs_to, and docs/skill-novelty.md describes the field.
| 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.
There was a problem hiding this comment.
Fixed in 0fdc281. The sentence split no longer breaks after e.g., i.e., vs. or cf., so "Profile code, e.g. PyTorch training loops, and report hotspots." stays one query. Pinned by test_a_sentence_with_e_g_or_i_e_is_one_description_query, which failed before the change.
|
|
||
| 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.
There was a problem hiding this comment.
Fixed in 0fdc281. The helper is public as repeat_noise in novelty/routing.py, and both modules import that name.
- --estimate-only refuses a run the real run would refuse (no query with a content word), with the same reason, and writes nothing. - The disabled-skill refusal reads the field once: the loader reports the names it left out, so field warnings no longer print twice. - Each all-vs-all noise row names its gold skill. - A sentence split does not break after e.g., i.e., vs. or cf. - _noise is public as repeat_noise: two modules use it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
🟡 Changes recommended
A lexical-only estimate incorrectly reports a query as sent when no selector calls occur because zero repeats still populate the prompt list.
1 open issue
What changed in this PR
1 new finding.
This PR extends dokimi’s routing evaluation with cost estimates, repeated all-vs-all measurements, and description-derived triage queries.
Changes:
- Adds single-artifact
--estimate-onlyand all-vs-all repeat, noise, and cost reporting. - Adds description fallback queries, clearer disabled-skill refusals, and precise seed help.
- Extracts routing reporting helpers, expands regression coverage, and updates documentation.
| File | Description |
|---|---|
dokimi/tests/test_novelty_routing.py |
Updates noise helper and no-query assertions. |
dokimi/tests/test_novelty_routing_field.py |
Checks disabled-skill warnings are not duplicated. |
dokimi/tests/test_novelty_routing_cli_gaps.py |
Covers estimates, description queries, and help text. |
dokimi/tests/test_novelty_routing_all_vs_all.py |
Covers repeats, costs, refusals, and noise attribution. |
dokimi/scripts/growth_baseline.json |
Lowers command-module growth baselines. |
dokimi/dokimi/novelty/routing.py |
Adds query derivation and estimation helpers. |
dokimi/dokimi/novelty/all_vs_all.py |
Pools repeated model results and labels noise rows. |
dokimi/dokimi/commands/novelty.py |
Wires CLI options and delegates reporting. |
dokimi/dokimi/commands/novelty_routing_report.py |
Centralizes query loading, estimates, refusals, and output. |
dokimi/docs/skill-novelty.md |
Documents the expanded routing behavior. |
dokimi/docs/reference/cli-interface.json |
Records the new estimate flag. |
dokimi/changelog.d/20261031_120000_napetrov_routing-cli-gaps.md |
Summarizes routing additions and fixes. |
Review effort: Balanced
| Every repeat sends the same prompts (see :func:`selection_prompt_tokens`). The reply is one | ||
| JSON pick, :data:`PICK_REPLY_TOKENS`. | ||
| """ | ||
| prompts = selection_prompt_tokens(candidates, queries) |
| f"({model_arm['first_error']}); the partial result, with those queries " | ||
| f"left out of every rate, is in {out}") |
There was a problem hiding this comment.
Must-fix: With --all-vs-all --repeats 2, one query can have a successful first call and a failed second call. _model_arm retains the successful call in the confusion matrix and own_rate, but this exit message says the query is left out of every rate. Describe failed calls as excluded from model-arm scoring, and distinguish that from repeat-noise rates, which exclude the entire query if any repeat fails. Update the matching warning in _cmd_routing_all_vs_all too, and cover a mixed-success repeat in a CLI test.
There was a problem hiding this comment.
Fixed in 12f8c62. The exit message and the WARNING now say failed calls are left out of model-arm scoring, and a query with a failed repeat is left out of the repeat-noise rates. A query's good repeat stays counted. Red test: test_a_repeat_that_fails_after_a_good_one_keeps_the_good_pick_in_model_scoring (new file, test_novelty_routing_repeat_failures.py) failed on the old wording and the old estimate.
…lls leave out --estimate-only without --model passed repeats=0, which still counted the prompts, so n_queries_sent was non-zero for a run that sends nothing. The failed-call message said the queries were left out of every rate. A call that failed is only left out of the model-arm counts; a query with a failed repeat is left out of the repeat-noise rates. Say both. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at 12f8c62. Three things are worth fixing before merge, plus one nit inline, and the merge waits for a decision on the description fallback.
@vlad-perevezentsev's blocking point on the description fallback is still open at this head: the fallback queries are sentences of the gold card's own description, so the rate is near 1.0 by construction. The author has handed the choice back and it waits for a decision. New evidence: the docs and the changelog still do not say a description run is a self-match, and skill-novelty.md puts the 0.225 corpus figure for self-derived queries next to the fallback text.
What I ran, on a Linux review host at 12f8c62:
make ci-local: 30 passed, 0 failed, 7 skipped; 11,996 tests; the changed-line coverage step passes.- A mutation run over the new code: 114 mutants, 38 of which pass the five routing test files. Most only change a message. Two decide reported values and are in the inline comments.
- Reproductions for the code findings, with the LLM transport mocked.
Findings that cannot be anchored inline:
- The "Cost of
--modelin all-vs-all" paragraph is stale after this PR.docs/skill-novelty.md:693-698says the model arm makes one call per query, that the call count is the number of queries with a content word, and quotes the stderr lineall-vs-all: --model makes N model calls, one per query, .... With--repeats 5the run makes five times that, and the code (novelty.py:394-396) multiplies byargs.repeatsand prints "one per query and repeat". A reader who sizes a budget from this paragraph under-counts by the repeat factor. Suggestion: say "one per query and repeat" and update the quoted line.
Checked, no issues
- Estimate vs. run: the estimate prices every query, contentless ones included, once per repeat, and refuses exactly when the real run refuses (no query has a content word).
--all-vs-all --estimate-onlyis a usage error. - All-vs-all repeats: groups are query-major and repeat-minor,
n_unscoredis multiplied by the repeats, and the noise rows pair with their gold throughzip(..., strict=True). The partial-failure exit prints noise and cost before it exits. - Disabled-skill refusal: it runs before any client is built, the field is loaded once, and
--include-disabledbypasses it. - CLI snapshot:
cli-interface.jsonmatches a fresh dump, and--estimate-onlyis the only entry added. All commits are signed off.
Bot points I checked that no longer apply at this head (4)
- Copilot's point on the lexical-only estimate reporting a query as sent does not apply: at this head
selection_estimatetakes no prompts at zero repeats, son_queries_sentandn_callsare 0. - exai-bot's point on pricing run-wide token totals as one completion does not apply: the estimate now prices each call's prompt and one pick separately, then multiplies by the repeats.
- exai-bot's point that the cost and noise lines are unreachable after a failed call does not apply: they print before the failure exit, and a clean run prints them after the attractors.
- exai-bot's point on the partial-failure wording does not apply: the exit message and the warning now say failed calls are left out of model-arm scoring, and a good repeat stays counted.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
|
|
||
| _SENTENCE_END = re.compile(r"(?<=[.!?])\s+") | ||
| # A period that ends an abbreviation, not a sentence: "e.g.", "i.e.", "vs.", "cf.". | ||
| _ABBREVIATION_END = re.compile(r"(?:^|\s)(?:e\.g|i\.e|vs|cf)\.$", re.IGNORECASE) |
There was a problem hiding this comment.
Nit: The abbreviation fix still splits after parenthesized abbreviations. description_as_queries("Profile code (e.g. PyTorch training loops) and report hotspots.") produces "Profile code (e.g." and "PyTorch training loops) and report hotspots.", rather than one query, because ( does not satisfy (?:^|\s). Recognize opening punctuation before these abbreviations and add this case to the regression test; otherwise common description wording becomes two incomplete routing requests.
There was a problem hiding this comment.
Fixed in 8174695. The sentence splitter's abbreviation check now accepts an opening bracket or quote before e.g., i.e., vs. and cf., so Profile code (e.g. PyTorch training loops) and report hotspots. is one query. The case is added to test_a_sentence_with_e_g_or_i_e_is_one_description_query, which failed before the fix.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to 94f84fa, which fix the nit from my review:
94f84fa:dokimi/commands/novelty.py:824,docs/skill-novelty.md:621and the changelog entry now say the list-priceexpected_usdinstead ofusd.
make ci-local at 94f84fa: 30 passed, 0 failed, 7 skipped.
Not approving yet. Still open: the three risks from my review; @vlad-perevezentsev on description fallback routes the gold card's own text, so its rates are near 1.0 by construction; unfixed, docs and changelog silent; @vlad-perevezentsev on keep the description fallback (stamped as a self-match) or drop it: the author handed the choice back.
… the estimate and the noise rows - "(e.g." and "(i.e." no longer end a sentence, so "Profile code (e.g. PyTorch training loops) and report hotspots." is one query. - A test records the prompts the run sends and checks each one's length against the estimate, so the two cannot drift apart unseen. - A test with one skill, two queries and two repeats checks that each noise row names its own query and its own gold skill. Thread: PRRT_kwDOVBg7g86yazxp Thread: PRRT_kwDOVBg7g86yauiU Thread: PRRT_kwDOVBg7g86yauiV Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The round changed --queries-from-headings: a skill with no usable heading now takes its description's sentences as triage queries (changelog bullet), so the door-check shape runs instead of refusing. The shape test still expected the refusal from intel/main. A skill with neither still refuses; test_novelty_routing_cli_gaps covers that case. Thread: PRRT_kwDOVBg7g86yauiV Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
The merge still waits on two points. The all-vs-all cost paragraph in skill-novelty.md was not updated for --repeats, and the description fallback is still undecided. The code since my last review is sound: the estimate-versus-run test and the per-query gold test were added, and the (e.g. …) splitter fix is right.
I agree with @vlad-perevezentsev's blocking point on the description fallback. At this head the fallback queries are still sentences of the gold card's own description, and neither skill-novelty.md nor the changelog says a description run is a self-match. The author handed the choice back, so it waits for a decision.
What I ran, on a Linux review host at 66341f4:
make ci-local: 30 passed, 0 failed, 7 skipped; the changed-line coverage step passes.- A mutation run over the two new tests and the splitter regex: 28 mutants. The estimate test catches the divisor, card numbering and run-side separator mutants. The survivors are small (no exact pin on
PICK_REPLY_TOKENS, no two-skill all-vs-all case pairing each noise query with its own gold), so I left them out.
Findings that cannot be anchored inline:
- The "Cost of
--modelin all-vs-all" paragraph is still stale.docs/skill-novelty.md:693-697says one call per query and quotesall-vs-all: --model makes N model calls, one per query, .... With--repeats 5the run makes five times that, andnovelty.py:396prints "one per query and repeat". A reader sizing a budget from this paragraph under-counts by the repeat factor. This is the point from my earlier review; the commits since did not touch it. Suggestion: say "one per query and repeat" and update the quoted line.
Checked, no issues
- Estimate vs. run: the new test compares each prompt the run sends with
selection_prompt_tokens. Mutating// 4to// 3or// 5, dropping the position prefix, or a\n\ncard separator in the run all fail it. - Noise
gold: the new test with one skill, two queries and--repeats 2fails when the gold list is one label per skill. - Splitter:
(e.g.,(i.e.andvs.stay inside one query; dropping the bracket class fails the test. - Commits: all 13 non-merge commits are signed off.
Bot points I checked that no longer apply at this head (4)
- Copilot's point on the lexical-only estimate reporting a query as sent does not apply: at this head
selection_estimatetakes no prompts at zero repeats, son_queries_sentandn_callsare 0. - exai-bot's point on pricing run-wide token totals as one completion does not apply: the estimate now prices each call's prompt and one pick separately, then multiplies by the repeats.
- exai-bot's point that the cost and noise lines are unreachable after a failed call does not apply: they print before the failure exit, and a clean run prints them after the attractors.
- exai-bot's point on the partial-failure wording does not apply: the exit message and the warning now say failed calls are left out of model-arm scoring, and a good repeat stays counted.
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 66341f4. The estimate refusal, the single field read, the gold on noise rows, the abbreviation split and the repeat_noise rename are all fixed and tested. The description fallback is still in, and I replied in its thread with my pick: drop it.
Agreed with @antonwolfy: the "Cost of --model in all-vs-all" paragraph in skill-novelty.md (lines 693-697) still says one call per query and quotes the old stderr line, while the run now makes one call per query and repeat.
What I ran, at 66341f4:
make ci-local --offline: 12,030 passed, 0 failed, 58 skipped, 6 xfailed. The one FAIL row isschema_compat, and it does not come from this PR. The PR touches no schema; the check reads the schemasmainadded after this branch's base.- My first-round repros again:
--estimate-onlyon an artifact with no usable query now exits with "no usable queries". A queries file that names a disabled catalog skill prints one--catalogcount. The description fallback still gives lexical R@1 1.0, MRR 1.0 withqueries_source: description. description_as_queriesone.g.,(e.g. …),vs.andI.E.: each stays in one query.
Checked, no issues
- Disabled names:
_named_competitorsandcatalog_competitorscollect the names they leave out on the first read, so the refusal does not load the field again. - Noise
gold: one label per sent query, matched to the noise rows withzip(..., strict=True), so a length mismatch raises instead of mislabelling. - Zero-repeat estimate and failure wording: a lexical-only estimate sends nothing, and the partial-exit message says a good repeat stays counted.
…is refused A skill with no usable heading is refused with no_queries again, as on main. Its description's sentences are not triage queries: each scores near 1.0 against its own card. Removes description_as_queries and self_derived_queries, the abbreviation split, the description help text, docs and changelog bullet. Restores test_skill_repo_shapes from main and adds a refusal test. Growth baseline lowered for novelty.py. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The schema_compat check compares against the PR base; this branch lacked the schema files main added since its base, so they showed as removed. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
napetrov
changed the title
The cost paragraph said one call per query and quoted the old stderr line; with --repeats the run makes queries x repeats calls and prints 'one per query and repeat'. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
Main already hinted at --include-disabled when a query named a disabled skill; the change is that the message now names each such skill. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
LGTM
I checked bb30d32. The description fallback is gone: a skill with no usable heading is refused with no_queries again, as on main. Everything raised in the earlier rounds is fixed, including the all-vs-all cost paragraph and the changelog wording.
Merge: after a rebase: it conflicts with current main (now with #328 in) in skill-novelty.md, commands/novelty.py, novelty/routing.py, growth_baseline.json and test_novelty_routing_field.py. #316 and #337 conflict with it too, in novelty.py and growth_baseline.json.
What I ran, at bb30d32:
make ci-local --offline: 12,514 passed, 0 failed, 58 skipped, 6 xfailed. The one FAIL row isschema_compat, which does not come from this PR: it touches no schema, and the check readsheadroom.v1and therepo_environmentchange thatmainadded after this branch's base.- The first-round repro: a skill with a three-sentence description and only
## Steps, run with--queries-from-headings, exits 2 with "no usable queries" and writesstatus: no_queries.
Checked, no issues
- Removal:
description_as_queries,self_derived_queriesand the sentence splitter are gone, and nothing still refers to them.queries_sourceis nowheadingsornone, and the help, the docs and the changelog say the same. - Shape test:
test_skill_repo_shapes.pyis back to the merge-base version, so it expects the refusal for the no-heading shape. - Docs: the cost paragraph counts calls per query and repeat, and it quotes the stderr line the run prints.
Keeps intel#328's field rules (spec errors and name conflicts leave the field) and intel#329's estimate-only, all-vs-all repeats and cost; the refusal of a query file naming a left-out skill now names each skill with every reason that applies. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com>
|
Merged main (1cc9663) after #328. The field keeps #328's rules: a spec error or a name with conflicting copies leaves it. When a query file names a skill the field left out, the refusal now names each skill with every rule that applies (disable-model-invocation, spec error, name conflict) and the flag that keeps it. Estimate-only, all-vs-all repeats and cost are unchanged. Suite, ruff, mypy, bandit and the benchmark pass on Linux. |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
LGTM. The description fallback is gone, the cost paragraph counts calls per query and repeat, and the refusal for a query that names a left-out skill gives each skill's rules. One nit inline.
What I ran, on a Linux review host at 95631dc:
make ci-local: 30 passed, 0 failed, 7 skipped; the changed-line coverage step passes.- Reproductions with the LLM transport mocked: the estimate's
n_callsagainst the calls a real run makes (single artifact and all-vs-all, repeats 1 and 3, contentless queries included) match; the all-vs-all stderr call count matches the calls made; a partial failure at--repeats 3prints the cost of the calls that ran before the exit. - A mutation run over the new code, about 50 mutants: the survivors only change a label or a defensive guard (
queries_source: headingsis not asserted,PICK_REPLY_TOKENSis not pinned, a disabled-only--competitorsskill is not asserted); none decides an exit code or a rate.
Checked, no issues
- Description fallback: removed. A skill with a description and no usable heading exits 2 with
no_queriesandqueries_source: none. - Refusal naming left-out skills: the field is read once;
--include-disabledand--include-invaliddrop only the rule they waive; a name that is also in the field is not refused. - Noise rows: each carries its
gold, paired withzip(..., strict=True). - Commits: every non-merge commit is signed off.
Bot points I checked that no longer apply at this head (4)
- Copilot's point on the lexical-only estimate reporting a query as sent does not apply: at this head
selection_estimatetakes no prompts at zero repeats, son_queries_sentandn_callsare 0. - exai-bot's point on pricing run-wide token totals as one completion does not apply: the estimate prices each call's prompt and one pick separately, then multiplies by the repeats.
- exai-bot's point that the cost and noise lines are unreachable after a failed call does not apply: they print before the failure exit, and a clean run prints them after the attractors.
- exai-bot's point on the partial-failure wording does not apply: the exit message and the warning say failed calls are left out of model-arm scoring, and a good repeat stays counted.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
* feat(routing): estimate-only, all-vs-all repeats and cost, description 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> * fix(novelty): routing estimate counts every sent query, all-vs-all refuses --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> * fix(novelty): no derivation warning when --queries-from-headings derives 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> * fix(novelty): attractor line says calls, not queries, when the model 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> * fix(routing): price each estimated call, not the run's total 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> * fix(novelty): print the cost and noise of a partial all-vs-all run 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> * test(routing): cover the estimate for a model with no list price 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> * fix(novelty): keep the all-vs-all cost line on success; ratchet for the 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> * fix(routing): answer review of estimate, gold and disabled-skill paths - --estimate-only refuses a run the real run would refuse (no query with a content word), with the same reason, and writes nothing. - The disabled-skill refusal reads the field once: the loader reports the names it left out, so field warnings no longer print twice. - Each all-vs-all noise row names its gold skill. - A sentence split does not break after e.g., i.e., vs. or cf. - _noise is public as repeat_noise: two modules use it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(routing): zero-repeat estimate sends no query; say what failed calls leave out --estimate-only without --model passed repeats=0, which still counted the prompts, so n_queries_sent was non-zero for a run that sends nothing. The failed-call message said the queries were left out of every rate. A call that failed is only left out of the model-arm counts; a query with a failed repeat is left out of the repeat-noise rates. Say both. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * Name the estimate's price key expected_usd in help, doc, changelog Signed-off-by: Anton Volkov <antonwolfy@gmail.com> * fix(routing): keep a parenthesised abbreviation inside its query; pin the estimate and the noise rows - "(e.g." and "(i.e." no longer end a sentence, so "Profile code (e.g. PyTorch training loops) and report hotspots." is one query. - A test records the prompts the run sends and checks each one's length against the estimate, so the two cannot drift apart unseen. - A test with one skill, two queries and two repeats checks that each noise row names its own query and its own gold skill. Thread: PRRT_kwDOVBg7g86yazxp Thread: PRRT_kwDOVBg7g86yauiU Thread: PRRT_kwDOVBg7g86yauiV Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * test(routing): the no-heading shape falls back to its description The round changed --queries-from-headings: a skill with no usable heading now takes its description's sentences as triage queries (changelog bullet), so the door-check shape runs instead of refusing. The shape test still expected the refusal from intel/main. A skill with neither still refuses; test_novelty_routing_cli_gaps covers that case. Thread: PRRT_kwDOVBg7g86yauiV Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * fix(routing): drop the description fallback; a skill with no heading is refused A skill with no usable heading is refused with no_queries again, as on main. Its description's sentences are not triage queries: each scores near 1.0 against its own card. Removes description_as_queries and self_derived_queries, the abbreviation split, the description help text, docs and changelog bullet. Restores test_skill_repo_shapes from main and adds a refusal test. Growth baseline lowered for novelty.py. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> * docs(novelty): count all-vs-all model calls per query and repeat The cost paragraph said one call per query and quoted the old stderr line; with --repeats the run makes queries x repeats calls and prints 'one per query and repeat'. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com> * docs(changelog): say what the disabled-skill refusal used to print Main already hinted at --include-disabled when a query named a disabled skill; the change is that the message now names each such skill. Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com> * Document null queries_source for --queries runs Signed-off-by: Anton Volkov <antonwolfy@gmail.com> --------- Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> Signed-off-by: Anton Volkov <antonwolfy@gmail.com> Signed-off-by: Alexander Rybkin <alexander.rybkin@intel.com> Co-authored-by: Anton Volkov <antonwolfy@gmail.com> Co-authored-by: Alexander Rybkin <alexander.rybkin@intel.com>
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_sourceisheadingsornone. A skill with no usable heading still refuses withno_queries, as on main; its description is not used as a query (a description query scores near 1.0 against its own card, so it is not a routing rate). 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 1126 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) ran on
aa9fcbeb: pass 2 found one blocker (the attractor line counted calls under the word "queries"), fixed there, and pass 3 found none. The description fallback was removed after that pass, in review of this PR; that change got no fresh-context pass, only the suite, ruff and mypy.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.[a-z0-9]+) splits non-English words and gives CJK no tokens. It predates this PR, andheadings_as_querieshas the same limit: a CJK-only heading 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.docs/reference/cli-interface.jsonis regenerated for the new option.🤖 Generated with Claude Code