Skip to content

fix(novelty): a capped run probes atoms spread over the whole input - #358

Open
antonwolfy wants to merge 10 commits into
intel:mainfrom
antonwolfy:feat/novelty-cap-spread
Open

antonwolfy wants to merge 10 commits into
intel:mainfrom
antonwolfy:feat/novelty-cap-spread

Conversation

@antonwolfy

@antonwolfy antonwolfy commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Settles #293, where novelty analyze under an atom cap measured only the opening of the artifact. The decisions are posted on the issue (comment): a per-atom stride with no seed; re-measure only where the cap binds; reword the shortening forfeit; sampling becomes cap-spread:<n>; on --include-resources, the same stride over the bundle's atoms.

On main, extract_atoms stops its batch loop once it holds cap atoms. A capped run probes the first n atoms in document order, and its rate describes that stretch of the input. With a fake extractor giving one atom per span, on a 61-span input at the triage cap of 25 (the shape of the dpnp root AGENTS.md in the issue):

extraction calls atoms kept spans they come from record
main 1 25 s0001, s0002, s0003 … s0025 reached: s0025, 25 of 61 spans, 40% of tokens
this PR 3 25 s0002, s0004, s0007 … s0060 sampled: {"atoms": 25, "of": 61, "rule": "cap-spread"}, reached: null

The second commit answers the review of this PR: the stride now runs in document order whatever order the extractor's reply uses, a cap below 1 is refused, the cap texts no longer claim the whole input was read when spans went unread, and the docs no longer promise every file a probe or the same sample from the same text.

The third commit answers the second review. A run whose atoms exactly meet the cap keeps every atom, since extraction no longer stops there. At e8fdf95 such a run still carried the cap's forfeits and spans.capped_atom_set: true; now it carries neither. The changelog now says when extraction.sampled is set.

The fourth commit answers the third review, in the docs only: like any fixed stride, the spread sample can fall in step with a repeated structure. With M = 100 and n = 25 it always takes atoms 2, 6, ..., 98, so an input of same-shaped four-atom sections with the new fact in the same place can give a sampled rate far from the whole input's, and re-extracting the same atoms gives the same sample. The docs now name that limit and say when to pass --all-atoms. The stride stays seedless, as D017 decided.

The last three commits answer the fifth review. --estimate-only counted extraction as one call per 12 atoms expected before the cap. For triage on 300 spans and 19,000 tokens that gave 69 extraction calls, but the run asks one call per 25-span batch, which is 12 calls. The estimate now counts the span batches. A cap below 1 is now refused before extract_atoms resets the caller's span.unread marks, not only before any call. The review also asked that spans, atoms.json and shortening be indexed from every extracted atom while only the sample is probed. That changes the atoms.json contents, the cost of shortening's oracle and the novelty gate's reading of spans.capped_atom_set, so it is tracked in #371, which D017 now links (9f8bdf8).

Changes

  • novelty/atoms.py: extract_atoms no longer stops at the cap. It extracts every batch, then keeps spread_sample(atoms, cap, key=_earliest_span(spans)): the atoms sorted stably by the input position of the earliest span each cites (document order; on a bundle run the files in bundle order: entry point, then resources by role and path), then the ones at floor((i + 0.5) * M / n) for i in 0..n-1. The sort is needed because a batch's reply lists its atoms in the model's order, which the prompt does not fix: a reply citing s0001, s0003, s0002 at cap 1 probed s0003 before the sort and probes s0002 with it. With M ≤ n, or no cap, every atom is kept, unsorted, in extraction order, so atoms.json of an uncapped run is as before. The selection has no RNG and no seed. sample_record fills ExtractionReport.sampled when the cap left atoms out. _cap_reach and the sent set are gone, and report.batches / report.calls count every batch. A cap below 1 raises ValueError before any model call, and before any change to the spans passed in (AnalyzeConfig(max_atoms=0) reached a zero-atom sample; the CLI's --max-atoms already took positive ints only). The default batch of 25 spans per extraction call is now the constant BATCH_SPANS. The complexity of extract_atoms drops from 16 to 14 (baseline lowered).
  • Both novelty analyze and novelty atoms call extract_atoms, so probe and shorten read the same sample from atoms.json. Shortening candidates and the faithfulness, carrier and conflict checks get only the sampled atoms, as before.
  • novelty/analyze.py: sampling is cap-spread:<n> for new runs whose cap left atoms out (extraction.sampled set), and full when the cap kept every atom. The cap warning is added when extraction.sampled is set. Nothing in the package parses the string: it is copied into the scorecard and printed in novelty.md, so an old cap-per-artifact:<n> artifact reads as before.
  • novelty/report.py: a new describe_sample. cap_warning(extraction) takes the extraction record and reads atom extraction read the whole input, and the atom cap probes 25 of the input's 61 atoms, spread over all of it (a stride of 2.44 atoms in document order): rates are estimated from that sample; pass --all-atoms to probe every atom. When extraction.unread_span_ids is not empty (a failed call, an empty or unparseable reply), the warning, the novelty.md header and the coverage: line say instead that the sample is spread over the atoms extracted from the spans that were read, and how many spans were not: atom extraction could not read 25 of the input's spans, and the atom cap probes 2 of the 25 atoms extracted from the spans that were read, spread over those (a stride of 12.5 atoms in document order; 25 of the input's spans were not read, see extraction.unread_span_ids): .... coverage_reach and _capped_at use sampled when it is present. For an old artifact with reached they keep their present wording.
  • novelty/tiers.py: under a cap that left atoms out, with shortening on, the forfeit is now shortening candidates are scored against a spread sample of <n> of the extracted atoms: a cut of a span with no sampled atom looks free. forfeits is not passed M. estimate_calls takes n_spans and counts ceil(n_spans / BATCH_SPANS) extraction calls. RunSettings.estimate passes the span count, so --estimate-only counts every extraction batch the run asks, while the probe terms stay at the cap. The triage case above (300 spans, 19,000 tokens) now projects 13 stage calls: 12 extraction batches and the need graph. On main it projected 3, and at 82d576a 70. Without n_spans, estimate_calls keeps the rule of the measured runs, whose extraction stopped at the cap: one call per 12 atoms.
  • A run counts as capped only when the cap left atoms out (extraction.sampled is set), not when its atom count reaches the cap. tiers.forfeits (and RunSettings.forfeits, tier_record, warnings) takes sampled in place of n_atoms. analyze passes extraction.sampled is not None, so the sampled forfeits and the spread-sample shortening forfeit leave the warnings and run_config.tier of a run that kept every atom. Before extraction (--estimate-only, the CLI's tier listing) sampled is None and the cap is still assumed to bind. spans.span_summary takes capped in place of atom_cap, from the same state, in analyze and in novelty atoms, so spans.capped_atom_set (read by the novelty gate and the subject report) is false at M = n.
  • schemas/novelty.v1.json, widened in place: an optional extraction.sampled (atoms, at least 1, of, rule, all required inside it). The sampling, batches, reached and conflict_coverage descriptions now say what new runs write, and sampled's names the sort and the read spans. capped_atom_set's says it is true only when the cap left atoms out, and that older runs set it when the atoms reached the cap. Old artifacts still validate (schema_compat passes, and the committed novelty.v1 fixture is unchanged).
  • commands/novelty.py: novelty atoms prints the new cap warning. The --all-atoms help says "Probe every extracted atom", since a capped run now extracts every atom too.
  • Docs: in docs/skill-novelty.md, the cap passage, the extraction table (sampled, reached), the forfeit list and the batches note. The cap passage says the stride runs over the concatenated atom list of the selected files, so a file with few atoms can get no probe. It says the same extracted atom list, not the same text, gives the same sample, and that reusing atoms.json with novelty probe --atoms probes identical atoms across runs. It quotes both forms of the cap warning. Its forfeit list says that a skill with no more atoms than the cap keeps every atom (extraction.sampled null, spans.capped_atom_set false) and drops the cap's forfeits. Its cost section now says the measured runs and the tier table predate the spread cap. Where the cap passage says the sample estimates the per-atom rate an uncapped run reports, it adds the fixed stride's periodic-order limit (the M = 100, n = 25 example) and says to pass --all-atoms for an artifact of repeated, same-shaped sections (a template per item) or a rate that decides something. docs/reviewing.md and docs/skill-novelty-walkthrough.md: at triage, --include-resources strides over the selected files' atoms, a file with few atoms may get no probe, a higher tier spreads more probes without promising each file one, and --all-atoms probes every extracted atom. Neither of those two says the sample estimates the whole input's rate, so the periodic-order limit is not repeated there. docs/skill-novelty-evidence.md has no tier profiles and is unchanged.
  • Research register: decision D017, accepted, "A capped novelty run probes atoms spread over the input". Idea I005, link to novelty: spread the atom cap across the artifact instead of its opening #293. It says which atoms are probed changes only above the cap, while extraction cost changes for every run whose cap was reached before its last batch (an exact-cap artifact included). Re-measuring those runs is left to follow-up work. Its rationale names the periodic-order limit of the stride and when to pass --all-atoms.
  • Changelog fragment (Changed), with the same limit in a sentence.

Risk

  • Capped results change for artifacts with more atoms than the cap: more than 25 at triage, 60 at standard, 120 at default/study. Their rates now estimate the whole input's per-atom rate, so they are not comparable with capped results written before this change. On an artifact of repeated, same-shaped sections the fixed stride can still miss that rate; the docs point to --all-atoms there. Runs below the cap are unchanged.
  • Cost: probing still stops at the cap, but extraction now reads the whole input. On the 61-span example that is 3 extraction calls instead of 1. A run whose atoms exactly meet the cap before its last batch also asks the remaining batches now. --estimate-only counts these extra calls, one per 25-span batch, but no measured run has checked that count yet. Each extra call is priced with the tier's per-call extractor profile, which was fitted on capped runs. The held-out calibration checks in tests/test_novelty_tiers.py now project those 12 runs the way they were measured (extraction stopping at the cap). The triage extractor token profile is unchanged.
  • Readers: extraction.reached is null on new runs. A consumer that used it to tell a capped run should read extraction.sampled, or atoms.sampling (cap-spread:<n>). The in-repo readers (report, headline, settings parity test) do. A capped atoms.json lists its sample in document order, which can differ from the extractor's reply order.
  • AnalyzeConfig(max_atoms=0) from the Python API now raises ValueError before any model call; it was accepted before.
  • A run whose atoms exactly meet the cap is no longer flagged capped: spans.capped_atom_set is false and the cap's forfeits are gone from its warnings and run_config.tier. The novelty gate therefore checks its dead-token share and can clear harmful_atoms at 0 conflicts, as for any run under the cap. Artifacts already written keep what they stored.
  • On a capped run, the span index (atom_ids, frac_carrying_nothing), atoms.json and shortening still see only the sampled atoms. The spans with no sampled atom are now spread over the whole input instead of forming the unread tail. The frac_carrying_nothing and spread-sample shortening forfeits say so. Indexing them from every extracted atom is novelty: a capped run indexes spans and writes atoms.json from the sample only #371.

Testing

On a Linux review host, at 3606282 (on main 41b3d8d):

  • make ci-local: 30 passed, 0 failed, 7 skipped. Skipped: external-catalog-smoke (needs docker and network), test-latest, docs.yml:deploy and pip-audit.yml:report (job conditions false on a PR), cli-surface / Package the result (an expression ci-local cannot evaluate), conda-forge-deps (no micromamba), and semgrep secrets (not installed). 12566 tests passed, 5 skipped and 6 xfailed; changed-line coverage against main was 100% (61 lines measured). The branch merges main twice (9e72b02 at ad734f5, ca507e9 at 41b3d8d), with no conflicts.
  • New tests, offline with a fake extractor:
    • the stride indices for M=61, n=25 ([1, 3, 6, 8, 10, 13, 15, 18, 20, 23, 25, 28, 30, 32, 35, 37, 40, 42, 45, 47, 50, 52, 54, 57, 59]), n=1 ([30]), M=n, M<n and no cap;
    • a capped run sends every batch (24 calls / 8 batches where main made 6 / 2);
    • the same extracted list gives the same selection;
    • sampling is cap-spread:<n> when the cap binds, and full when the atoms meet or fall under it (10 spans under cap 25);
    • the warning, header and coverage texts of a spread run, and an old cap-per-artifact artifact with reached still validates and renders its old header and coverage line;
    • the forfeit text;
    • a bundle run strides across SKILL.md and references/tuning.md in file order;
    • novelty atoms --max-atoms 2 saves the same sample and asks both batches;
    • shortening candidates are built from the sampled atoms only.
  • Added for the review:
    • out-of-order replies: s0001, s0003, s0002 at cap 1 probes s0002; s0001, s0003, s0002, s0004 at cap 2 probes s0002 and s0004; an atom citing (s0004, s0001) sorts by s0001; at the cap and with no cap, reply order is kept;
    • a cap of 0 raises ValueError and the mocked client is never called;
    • the schema rejects extraction.sampled.atoms: 0;
    • 50 spans, cap 2, the first batch giving 25 atoms and the second an empty reply: the warning, the header and the coverage line name the 25 unread spans and make no whole-input claim.
  • Added for the second review:
    • a study run (shortening on) whose two atoms meet a cap of 2 has neither sampled forfeit and no spread-sample shortening forfeit, in its warnings or in run_config.tier, and spans.capped_atom_set false; at a cap of 1 it has all three;
    • novelty atoms over three one-atom spans prints capped_atom_set false at --max-atoms 3 and true at 2;
    • forfeits with sampled=False has no sampled or shortening forfeit, with sampled=True or before extraction it has both (the shortening-forfeit test no longer expects the warning at M = n).
  • Added for the fifth review:
    • a capped triage estimate counts one extraction call per 25-span batch: 12 for 300 spans and 19,000 tokens, 2 for 26 spans, and 1 for 25 spans (stage calls = batches + the need graph);
    • a cap of 0 leaves the caller's spans' unread marks as they were.
    • Against 82d576a the 300-span and 26-span cases fail (70 and 2 stage calls where 13 and 3 are expected), and so does the cap-0 test (the unread mark reset to false). The 25-span case passes on both, as it should: there the old rule happens to give the same count. The estimate test of study fails there on the n_spans keyword.
  • The third review changed docs only (no code or test), so it adds no test or mutant. tests/test_docs_code_refs.py and the comment-block check pass on it.
  • Against main's code, the first commit's new tests fail. Extraction makes 1 call where 6 are expected, (6, 2, 2) where (24, 8, 8) is expected, novelty atoms asks 1 batch, there is no sampled record, and the texts, forfeit and estimate are the old ones. The test of an old artifact validating passes on both, as it should. Against the first commit, the review's new tests fail: the three out-of-order cases, the cap of 0, the zero-atom schema case and the partly-read case. The at-cap and no-cap cases pass on both, as they should: they pin the order a cap that does not bind keeps. Against the second commit, the at-cap cases of the second review's tests fail (the forfeits and capped_atom_set: true at M = n), and the cap-binds cases pass, as they should. The forfeits test fails there on the keyword it now passes.
  • Mutants, each in a copy of the commit, each caught by every test aimed at it (40 of 40):
    • the stride starting at each stride's first atom;
    • the cap keeping the first n atoms;
    • the early stop restored;
    • a sample recorded at M = n;
    • no sample recorded;
    • cap-per-artifact restored;
    • the cap warning dropped in analyze, and in novelty atoms;
    • "spread over all of it" dropped from the sample text;
    • the coverage line and the report header ignoring sampled;
    • the old header and coverage line losing their reach;
    • the old shortening forfeit restored;
    • the estimate counting extraction over the capped atoms;
    • the schema no longer requiring sampled.of;
    • shortening built from more than the sampled atoms;
    • the document-order sort removed; the sort key the first cited span instead of the earliest; a cap that does not bind sorted too;
    • the cap check removed, and moved after the extraction calls;
    • the schema's minimum of 1 on sampled.atoms removed;
    • the unread case of the sample text, and of the "what extraction read" phrase, falling back to the whole-input text; cap_warning, coverage_reach and _capped_at each ignoring unread spans; a fully read input described as partly read;
    • equality counted as capped again: the atom count in place of the sampling state in analyze's warnings, its run_config.tier and its span_summary, and in novelty atoms' span_summary; forfeits ignoring sampled; span_summary ignoring capped;
    • the estimate not passing the span count (the atom rule again); the batches rounded down; the batches counted from the atoms expected before the cap; BATCH_SPANS at 20 instead of 25;
    • the cap check moved back after the loop that resets span.unread.
  • tests/test_novelty_settings_parity.py compares against main's capture. It now records the intended changes the way it already does: the forfeit and cap-warning texts, stage_calls in the estimate, and for the two --max-atoms 1 runs only the settings, since they now probe a different atom.
  • Not run: any real model. The tier profiles were measured with document-order extraction; re-measuring the capped runs is the follow-up D017 names.
  • Merged together at 82d576a with the branches for review_replay: optional judged recall over the credited hits #291 (judged recall, 8e05118), repo compare: flag replay tasks a guidance revision may have seen #290's recording half (f6a9cb9) onto main 41b3d8d: the full suite (12641 passed, 5 skipped, 6 xfailed), mypy (318 files) and ruff check pass.
  • Not yet run merged together with the other follow-up branches at 3606282.

Revert

git revert the eight non-merge commits. New artifacts written with cap-spread:<n> and extraction.sampled stay valid novelty.v1 once reverted (extraction takes extra keys), but render without the sample sentence. D017 would need a superseded record rather than deletion.

Checklist

  • Tests added/updated and pytest passes
  • ruff, mypy, and schema checks pass
  • Docs updated (or N/A) — see the docs-drift checklist in CONTRIBUTING.md
  • Self-reviewed per "Before opening a PR" in AGENTS.md
  • No ad-hoc run artifacts committed
  • New files carry the SPDX license header (or are exempt in .github/.licenserc.yaml)

Settles intel#293. A capped novelty run took atoms in document
order and stopped extraction at the cap, so its rates described the
opening of the input (at triage, about a quarter of the root AGENTS.md
of the dpnp guidance) and moved when sections were reordered.

Extraction now reads every span. Of the M atoms extracted, a cap of n
keeps those at floor((i + 0.5) * M / n), in extraction order (document
order; on a bundle run, the files in bundle order), via the new
atoms.spread_sample. There is no seed: the same extracted list gives the
same sample. With M <= n every atom is kept, as before. novelty analyze
and novelty atoms both go through extract_atoms, so probe and shorten
read the same sample from atoms.json.

- sampling is cap-spread:<n> for new runs; extraction.sampled records
  {"atoms", "of", "rule"} and extraction.reached is null. novelty.v1 is
  widened in place: older cap-per-artifact artifacts with a reach still
  validate and render as before.
- The cap warning, the novelty.md header and the coverage: line say how
  many of the input's atoms were probed and at what stride.
- The shortening forfeit says candidates are scored against a spread
  sample of the cap's atoms, so cutting a span with no sampled atom
  looks free.
- --estimate-only counts every extraction batch over the expected
  uncapped atoms; probing stays at the cap. The held-out calibration
  checks still project the runs as they were measured.
- Decision D017 records the rule and leaves re-measuring the capped
  tier profiles to follow-up work. Docs and a changelog fragment
  (Changed) follow the code.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Copilot AI balanced review requested due to automatic review settings November 1, 2026 19:28
@antonwolfy
antonwolfy requested a review from a team as a code owner November 1, 2026 19:28
@exai-bot

exai-bot commented Nov 1, 2026 •

Copy link
Copy Markdown

✅ OpenCodeReview: Review complete: 0 finding(s) across 20 selected item(s).

Review details
  • Commit reviewed: 9f8bdf8 against merge base 41b3d8d
  • Coverage: 20 files in 4 groups
  • Engine: OpenCodeReview v1.13.3, us.openai.gpt-6.1-sol
  • Usage: 338k tokens (148k from cache), 28s
  • Tool calls: 29 (23 file_read, 5 code_search, 1 file_read_diff)
  • Novelty behavior documentation and design decision: 5 files
  • Novelty atom, span, and tier scoring with unit tests: 6 files
  • Novelty analysis, settings parity, and baseline: 4 files
  • Novelty CLI, report output, and schema: 5 files

Comment thread dokimi/docs/reviewing.md Resolved
Comment thread dokimi/docs/skill-novelty.md Resolved
Comment thread dokimi/docs/skill-novelty.md Resolved
Comment thread dokimi/dokimi/novelty/report.py Resolved
Comment thread dokimi/dokimi/novelty/report.py Resolved

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The schema accepts zero-atom samples that crash rendering, and capped sampling uses extractor reply order instead of document order.

2 open issues
What changed in this PR

2 new must-fix findings.

This PR addresses #293 by changing capped novelty runs from measuring the input’s opening to selecting a deterministic spread of extracted atoms across the input.

Changes:

  • Extract every batch before applying the atom cap, including across bundled resources.
  • Record and report spread sampling while preserving support for older artifacts.
  • Update cost estimates, regression tests, documentation, and decision D017.
File Description
dokimi/​tests/​test_novelty_tiers.py Tests revised forfeits and full-extraction estimates.
dokimi/​tests/​test_novelty_settings_parity.py Accounts for intentional sampling and estimate changes.
dokimi/​tests/​test_novelty_report.py Tests spread reporting and legacy rendering.
dokimi/​tests/​test_novelty_cli.py Tests the sampled-coverage headline.
dokimi/​tests/​test_novelty_atoms.py Tests deterministic selection and extraction of every batch.
dokimi/​tests/​test_novelty_analyze.py Tests sampling metadata, bundles, and shortening inputs.
dokimi/​scripts/​growth_baseline.json Lowers the extraction complexity baseline.
dokimi/​research/​records/​decision/​D017.md Records the sampling decision and calibration follow-up.
dokimi/​dokimi/​schemas/​novelty.v1.json Adds optional sample metadata and updates descriptions.
dokimi/​dokimi/​novelty/​tiers.py Updates shortening forfeits and extraction estimates.
dokimi/​dokimi/​novelty/​report.py Renders sample details while retaining legacy reach text.
dokimi/​dokimi/​novelty/​atoms.py Replaces early stopping with post-extraction spread selection.
dokimi/​dokimi/​novelty/​analyze.py Records the new sampling rule and warning.
dokimi/​dokimi/​commands/​novelty.py Updates CLI warnings and --all-atoms help.
dokimi/​docs/​skill-novelty.md Documents sampling, metadata, costs, and compatibility.
dokimi/​docs/​skill-novelty-walkthrough.md Explains capped sampling across bundled files.
dokimi/​docs/​reviewing.md Updates resource sampling and cost guidance.
dokimi/​changelog.d/​20261101_120000_antonwolfy_novelty-cap-spread.md Announces the changed capped-run behavior.

Review effort: Balanced

Comment thread dokimi/dokimi/novelty/atoms.py Resolved
Comment thread dokimi/dokimi/schemas/novelty.v1.json Resolved
@antonwolfy antonwolfy self-assigned this Nov 1, 2026
Settles the review of the cap-spread change (intel#293):

- When the cap binds, the atoms are sorted (stably) by the input
  position of the earliest span each cites before the stride is taken,
  so the sample is in document order whatever order the extractor's
  reply lists a batch's atoms in. A cap that does not bind keeps the
  extraction order, so atoms.json is unchanged for those runs.
- extract_atoms refuses a cap below 1 with ValueError before any call,
  and extraction.sampled.atoms has a minimum of 1, so no record asks
  the renderers to describe a stride over zero atoms.
- When spans went unread, the cap warning, the novelty.md header and
  the coverage line say the sample is spread over the atoms extracted
  from the spans that were read and how many spans were not, instead of
  claiming the whole input was read. cap_warning takes the extraction
  record.
- The docs and D017 say the stride runs over the concatenated atom list
  of the selected files, so a file with few atoms may get no probe;
  that the same extracted atom list, not the same text, gives the same
  sample, and that reusing atoms.json probes identical atoms; and that
  extraction cost changes for every run whose cap was reached before
  its last batch, an exact-cap artifact included.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Comment thread dokimi/dokimi/novelty/tiers.py Resolved
@codecov-commenter

codecov-commenter commented Nov 1, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.32%. Comparing base (41b3d8d) to head (9f8bdf8).
⚠️ Report is 3 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #358      +/-   ##
==========================================
+ Coverage   94.29%   94.32%   +0.02%     
==========================================
  Files         308      309       +1     
  Lines       39322    39460     +138     
  Branches     6651     6676      +25     
==========================================
+ Hits        37079    37220     +141     
  Misses       1430     1430              
+ Partials      813      810       -3     
Files with missing lines Coverage Δ
dokimi/dokimi/commands/novelty.py 95.32% <100.00%> (ø)
dokimi/dokimi/novelty/analyze.py 95.45% <100.00%> (+0.59%) ⬆️
dokimi/dokimi/novelty/atoms.py 98.22% <100.00%> (+0.01%) ⬆️
dokimi/dokimi/novelty/report.py 98.77% <100.00%> (+0.09%) ⬆️
dokimi/dokimi/novelty/spans.py 92.90% <100.00%> (ø)
dokimi/dokimi/novelty/tiers.py 100.00% <100.00%> (ø)

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Settles the two review comments on e8fdf95 in intel#358 (issue intel#293).

Extraction now reads every span, so a run whose atoms exactly meet the
cap keeps every atom and records `extraction.sampled` as null. Two
places still read `n_atoms >= cap` as capped:

- `tiers.forfeits` took the post-cap atom count, so such a run carried
  the `sampled` forfeits and the spread-sample shortening forfeit in
  its warnings and `run_config.tier`. It now takes `sampled`, whether
  the cap left atoms out; `analyze` passes `extraction.sampled is not
  None` to `settings.warnings` and `tier_record`. Before extraction
  (estimates, the CLI's up-front tier listing) `sampled` is None and
  the cap is still assumed to bind.
- `spans.span_summary` set `capped_atom_set` from the same count. It
  now takes `capped` from the same state, in `analyze` and in
  `novelty atoms`. Artifacts already written keep what they stored.

The `capped_atom_set` schema description and docs/skill-novelty.md say
when the flag and the forfeits apply. The changelog fragment says
`extraction.sampled` is the {"atoms", "of", "rule"} object only when
the cap leaves atoms out, and null when it keeps every atom.

Tests: a study run whose atoms meet the cap has no sampled or
shortening forfeit and `capped_atom_set` false, one atom over the cap
has all three; `novelty atoms` flags the set the same way; the
shortening-forfeit test no longer expects the warning at equality.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Comment thread dokimi/docs/skill-novelty.md Resolved
Settles the review point on intel#358 that a spread sample's
estimate of the whole input's per-atom rate went unqualified. A fixed
stride can fall in step with a repeated structure: with M = 100 atoms
and a cap of 25 it always takes atoms 2, 6, ..., 98, so an input of
same-shaped four-atom sections, each with its new fact in the same
place, can give a sampled rate far from the whole input's, and
re-extracting the same atoms gives the same sample.

The cap passage of docs/skill-novelty.md, the changelog fragment and
decision D017's rationale now say so, and recommend --all-atoms for an
artifact of repeated, same-shaped sections or a rate that decides
something. The stride stays seedless, as D017 decided; no code changes.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Bring in main at ad734f5 (intel#339, a SCIP code index reader; intel#340, eval coverage against a frozen need graph). No conflicts. CI's schema_compat compares the branch with main, which gained eval_coverage.v1 and need_graph.v1 after this branch's base.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
@antonwolfy

Copy link
Copy Markdown
Contributor Author

Review rounds at 9e72b02, one commit per round, each thread answered and resolved:

  • e8fdf95: under a binding cap the stride runs in document order (atoms sorted by their earliest cited span), whatever order the extractor's reply uses. A cap below 1 is refused before any call, and sampled.atoms has a minimum of 1. When spans went unread, the cap warning, the header and the coverage line no longer claim the whole input was read. The docs no longer promise every file a probe, and they say the same extracted atom list, not the same text, gives the same sample.
  • c49b221: a run counts as capped only when the cap left atoms out. A run whose atoms exactly meet the cap has no cap forfeits and capped_atom_set: false, in tiers.forfeits and in spans.span_summary.
  • f9f9a8a: the docs, the changelog and D017 name the fixed stride's periodic-order limit, and say when to pass --all-atoms.
  • 9e72b02: merges main at ad734f5, with no conflicts. CI's schema_compat compares the branch with main, which gained eval_coverage.v1 and need_graph.v1 after this branch's base; c49b221 had failed that step for this reason only.

make ci-local is green at 9e72b02 (30 passed, 0 failed). With #357 and #359 at their heads, merged together onto main d3e2a26, the full suite (12331 passed), mypy and ruff pass. The description is updated.

@napetrov napetrov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: no blocking finding; one nit. Posted as a comment, not an approval. Head 9e72b02, merge base ad734f5, lane L2.

# Finding Label Provenance Probe
1 an uncapped run (M <= N) reads cap-spread:N in atoms.sampling, the label the Risk section names as the capped-run signal (C7) nit pre-existing (base labels it too) 10 spans, cap 25, head vs base

Earlier bot threads (exai-bot T1 to T11, Copilot T7 and T8) are fixed on head, except the docs wording which I read but did not re-probe line by line. The zero-atom division and the reply-order sort were probed.

Not probed here: the extraction-batch projection in the estimate (round(extracted / 12)) against the real 25-span batches; the PR says no measured run has checked it. The sampled forfeit text in tiers still says the rate is "not the whole input", which the new docs contradict (wording only). The litellm-dependent tests did not run on this host; CI on head is green.

Coverage: passes at level high (plain, edge, dedupe, VERIFY on 4 candidates); probes on head and base for the label, the unread-span path, and the sample table; schema minimum; prior threads. Not looked at: the CLI end to end; every docs line.

Comment thread dokimi/dokimi/novelty/analyze.py Resolved
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Review of intel#358 (comment 4438896876): `atoms.sampling` came from
`max_atoms` alone, so a run whose atoms met or fell under the cap
(`extraction.sampled` null, every atom probed) still read
`cap-spread:<n>`. A run with 10 atoms under the triage cap of 25 was
labelled `cap-spread:25`.

`analyze` now derives the label from the same state as the forfeits and
`spans.capped_atom_set`: `cap-spread:<n>` when `extraction.sampled` is
set, `full` otherwise. Readers of the label only print or copy it, so
such a run renders as a full one. The schema description, the cap
passage of docs/skill-novelty.md and the changelog fragment say when
each label is written.

The review also named the `sampled` forfeit, which called `added` a
rate over "the atoms the cap kept, not the whole input", although a
spread run reads every span. It now says the rate is over the sample of
extracted atoms the cap kept, not over every atom; the docs quote and
the settings-parity test follow.

Tests: the cap-met run is labelled `full`; a 10-span input under cap 25
is labelled `full` with `sampled` null and renders "10 atoms (full)";
the cap-met/cap-binds pair pins both labels.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
@antonwolfy

Copy link
Copy Markdown
Contributor Author

Review round at 82d576a, on top of a merge of main at 41b3d8d (ca507e9, clean):

  • 82d576a: atoms.sampling is cap-spread:<n> only when the cap left atoms out (extraction.sampled set), and full when the atoms meet or fall under the cap. The sampled forfeit now says added is a rate over the sample of extracted atoms the cap kept, not over every atom, since a spread run reads every span.

CI is green on 82d576a. With #357 and #359 at their heads, merged together onto main 41b3d8d, the full suite (12641 passed), mypy and ruff pass.

Comment thread dokimi/dokimi/novelty/tiers.py Resolved
Comment thread dokimi/dokimi/novelty/atoms.py Resolved
Comment thread dokimi/dokimi/novelty/atoms.py Resolved
@vlad-perevezentsev

Copy link
Copy Markdown
Contributor

Review at 82d576a. The sampling change is right: the stride runs in document order, a cap that keeps every atom counts as a full run everywhere (label, forfeits, capped_atom_set), and old cap-per-artifact artifacts still validate and render. Two things are left: the --estimate-only extraction count, and the full atom list that extraction now pays for and then drops. Both are in the line comments.

What I ran, at 82d576a:

  • make ci-local --offline: 19 passed, 0 failed, 13 skipped (the skips need network).
  • resolve_settings("triage", {}).estimate(300, n_tokens=19000): 70 stage calls, 69 of them extraction, where the run asks 12 batches.

Checked, no issues

  • Earlier threads: all 12 are fixed in the code: zero cap refused and sampled.atoms minimum 1, document-order sort, unread spans in the warnings and header, the full label at M <= n.
  • Schema: novelty.v1 only widens (new optional extraction.sampled, new descriptions), so artifacts already written still validate.
  • Docs and changelog: the stride, its periodic-order limit and the bundle caveat match the code; D017 has the license header.

Comments on lines outside the diff:

  • dokimi/dokimi/novelty/atoms.py:387-389: Nit: This comment still says the halves are re-asked in place "so atoms keep document order", but under a binding cap the order now comes from the sort by earliest span. In place still keeps batches in order, which matters when the cap keeps every atom. Suggestion: "so batches keep document order", as the docstring now says.

extract_atoms checked the cap after the loop that resets every span's
unread mark, so a cap below 1 raised before any model call but after it
had changed the caller's spans. The check now runs first, and the cap-0
test asserts the spans keep the unread marks they came with.

Settles the review comment on intel#358 that _check_cap runs
after the side effect it is meant to precede.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
--estimate-only counted extraction as one call per 12 atoms expected
before the cap, and the token rule expects one atom per 23 tokens. For
triage on 300 spans and 19,000 tokens that gave 69 extraction calls,
while the run asks one call per 25-span batch: 12.

estimate_calls now takes n_spans in place of n_extracted and counts
ceil(n_spans / BATCH_SPANS) extraction calls, with BATCH_SPANS the
default batch size of extract_atoms, now a shared constant.
RunSettings.estimate passes the span count. Without n_spans the old
per-12-atoms rule stays, which is how the held-out calibration runs,
whose extraction stopped at the cap, are projected. The per-call token
profiles are unchanged.

Settles the review comment on intel#358 that the estimate counts
extraction calls from the pre-cap atom guess.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
…ample

A capped run extracts every span but indexes spans, writes atoms.json
and shortens from the sampled atoms only. D017 now names that gap and
links the issue that tracks it.

Settles the review comment on intel#358 that asked for the issue
to be linked from D017.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
@antonwolfy

Copy link
Copy Markdown
Contributor Author

Round for @vlad-perevezentsev's review, at 9f8bdf8 (CI green):

  • 3606282: --estimate-only counts extraction as one call per batch of 25 spans (BATCH_SPANS, shared with extract_atoms), not one per 12 atoms expected before the cap. Triage on 300 spans / 19,000 tokens: 12 extraction calls, was 69.
  • 285e29e: a cap below 1 is refused before extract_atoms resets the caller's span.unread marks.
  • 9f8bdf8: D017 links novelty: a capped run indexes spans and writes atoms.json from the sample only #371, the follow-up for indexing spans, atoms.json and shortening from every extracted atom while only the sample is probed (atoms.json contents, shortening's oracle cost and the gate's capped_atom_set all change, so it is its own PR).

@napetrov napetrov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 9f8bdf8: CI green, my earlier threads are addressed, and re-review of the latest round found nothing in this PR's files.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants