Repository navigation
Conversation
…pare utils.normalize_model_ref (baseline check, self-judge warning) and repos/report.model_matches (repo compare) disagreed on dated ids, regional profiles, provider routes, -vN:M revisions and case. Replace both with utils.same_model: route, region profile, revision and case are not identity; a dated id matches its undated alias; two dated ids match only on the same date. Refs intel#229 (P2-3). Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #273 +/- ##
=======================================
Coverage 93.48% 93.49%
=======================================
Files 272 272
Lines 35363 35388 +25
Branches 6001 6009 +8
=======================================
+ Hits 33060 33085 +25
Misses 1489 1489
Partials 814 814
🚀 New features to boost your workflow:
|
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at e159659. One same_model helper for the self-judge warning, baseline compare and repo compare is the right direction, and the panel pairing is correct. But dokimi run still stores the self-judge flag by the old rule. The looser rule also lets baseline compare --strict pass two cases it refused before: an undated alias and a different org. Details are in the line comments.
What I ran, at e159659:
make ci-local --offline: 0 failed, 17 passed, 13 skipped (network/docker jobs).same_modelon edge cases: alias vs dated snapshot (True),meta-llama/Xvsmyorg/X(True),openai.gpt-oss-120b-1:0vsgpt-oss-120b(False),gpt-4o-2024-05-13vsgpt-4o-2024-08-06(False).
Checked, no issues
- Panel matching:
_same_recordedpairs the judges one to one by augmenting paths, so a non-transitive match (alias vs two snapshots) cannot pair two judges with one. The tests cover unequal sizes and the two-snapshot case. repo compare/repo report: the oldmodel_matchescases intest_a_provider_prefix_is_not_another_modelstill pass undersame_model, and thecjm-model-answeredoutcome test is only renamed.- Bedrock aliases: the
BEDROCK_MODEL_IDSentries map a bare name tous.anthropic.<same name>, so dropping the lookup loses no match. - Docs and changelog: the
baseline comparesection and the fragment describe the new rule. No scores change.
| some snapshot of it), or the same id behind another route or region profile. Two dated | ||
| ids match only on the same date, and a bare family name (``haiku``) is not a full id. | ||
| The provider is not part of the identity: the same weights on Bedrock and on the direct | ||
| API are still one model for the self-judge warning and for a baseline's model check. | ||
| """ | ||
| prov, name = (provider or "").strip().lower(), (model or "").strip() | ||
| if prov == "amazon-bedrock": | ||
| name = BEDROCK_MODEL_IDS.get(name, name) | ||
| return f"{prov}/{name.lower()}" | ||
| (base_a, date_a), (base_b, date_b) = _model_core(a), _model_core(b) | ||
| if not (base_a and base_b) or (date_a and date_b and date_a != date_b): | ||
| return False | ||
| # a lowercase word before the first dot is taken as a vendor prefix (``anthropic.``, ``amazon.``) and | ||
| # dropped; a dot after a digit or a dash (``gpt-4.1``, ``qwen2.5``) is part of the name | ||
| return re.sub(r"^[a-z]+\.", "", base_a) == re.sub(r"^[a-z]+\.", "", base_b) |
There was a problem hiding this comment.
baseline compare --strict now passes when one side records only an undated alias, so a judge or answer model change behind the alias is no longer caught.
same_model('anthropic/claude-sonnet-4-5-20240620', 'claude-sonnet-4-5') is True. The alias does not record which snapshot answered, so this is a role that was not measured, and --strict now reads it as a match. Before this PR it was a mismatch. For the self-judge warning a loose rule is the safe side (it warns more), but for baseline compare it is the unsafe side (it refuses less).
Suggestion: keep the alias-matches-snapshot rule for the warning and repo compare, but in baseline compare treat dated vs undated as "not verified" (named on stderr, refused by --strict), the same way a role that is not recorded is handled.
There was a problem hiding this comment.
Waiting on a decision before I change this: the undated alias matching its dated snapshot was a deliberate part of this PR, but you are right that before it baseline compare --strict refused that pair, and that this is the unsafe direction for a gate. Options: (a) keep one rule everywhere, as now; (b) as you suggest, keep the alias match for the warning and repo compare, and make baseline compare read dated against undated as "not verified" (named on stderr, refused by --strict). (b) also means a panel in baseline compare pairs by equality of the dated or undated form, so the augmenting-path pairing and the panel cases added in 8a2f952 would change. My recommendation: (b), since --strict is a gate and refusing more is the safe side. The thread stays open until then.
There was a problem hiding this comment.
(b), as you recommend. The panel pairing does not have to go. Compare in two passes: first by exact normalized id and date (for a panel: the same multiset). If that matches, it is a match. If it fails but same_model pairs everything (your augmenting-path pairing, unchanged), the role is "not verified": it is named on stderr and --strict refuses it. Anything else is a mismatch. So a dated-vs-undated pair is never a match and never a false mismatch. The m,m-20240101 vs m-20240101,m-20250101 cases then expect "not verified" instead of a match.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at e159659. The single same_model rule is the right direction, and the augmenting-path panel pairing is correct. Two test gaps in the panel pairing are worth fixing before merge, and @vlad-perevezentsev's open points still hold.
I agree with @vlad-perevezentsev's points on the stored evaluator_independence_warning in dokimi run, the undated alias passing baseline compare --strict, the dropped org in rsplit, openai.gpt-oss-120b-1:0, and the docstring wrap. I reproduced each at this head.
What I ran, on a Linux review host at e159659:
make ci-local: 27 passed, 1 failed, 7 skipped. The failure ispip-audit, which listsfilelockadvisories on the lock files; this PR does not touch them.- 9,758 tests passed, changed-line coverage 100%.
- Reproductions for the code findings, with the LLM transport mocked, and mutations of
_same_recorded.
Checked, no issues
- Panel/non-panel and one-to-one pairing: removing the panel check, dropping
tried.add,anyforalland==forsame_modelare all caught by the suite. repo compare/repo report:same_modelis symmetric, and each loop in_models_equivalentcovers one direction.- Changelog and growth ratchet: the fragment is in place and
ci-localpasses both.
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 14 selected item(s). Review details
|
…odel-match-rule Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com> # Conflicts: # dokimi/dokimi/repos/compare.py # dokimi/dokimi/repos/report.py
The pipeline compared provider and model as exact strings, so the printed warning and run_metadata.evaluator_independence_warning disagreed for one model on two routes. Use same_model, as eval does. Thread: PRRT_kwDOVBg7g86xgsTm Thread: PRR_kwDOVBg7g88AAAABU4ae8g Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Only a known route (provider or gateway prefix, an ARN's inference-profile/) is dropped; the rest of the path stays in the id, so meta-llama/X and myorg/X are two models. Thread: PRRT_kwDOVBg7g86xgsT1 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
openai.gpt-oss-120b-1:0 did not match gpt-oss-120b, so the self-judge warning missed it and repo compare reported a change that did not happen. Thread: PRRT_kwDOVBg7g86xgsT- Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
gpt-4o-2024-08-06 did not match gpt-4o, so the self-judge warning was skipped and a baseline check reported a false model mismatch. A dashed and a compact date of one day compare equal. Thread: PRRT_kwDOVBg7g86xlsyt Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The edited line was 123 characters and the next one started at column 0. Thread: PRRT_kwDOVBg7g86xgsUD Thread: PRRT_kwDOVBg7g86xlsyi Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
A baseline panel of two judges against a current panel of three, where every baseline judge pairs, was accepted when the size check was deleted. Thread: PRRT_kwDOVBg7g86xiXNo Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Baseline m,m-20240101 against current m-20240101,m-20250101 is the same panel, and only an augmenting path finds the pairing: a first-match pairing strands the second judge. Replacing the augment step with a first match left the suite green; the new case, in both orders, fails it. Thread: PRRT_kwDOVBg7g86xiXN0 Thread: PRRT_kwDOVBg7g86xlsy9 Thread: PRR_kwDOVBg7g88AAAABU5SrZA Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The assertions checked only the return value. A warning printed for every pair passed the old test; the new one fails it. It also covers an unset answer or judge model, which must not warn. Thread: PRRT_kwDOVBg7g86xlszG Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…del id Thread: PRRT_kwDOVBg7g86xgsT1 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… nine Found by the self-review of this round: with the organization kept, a route missing from the list (azure/, gemini/, groq/, models/, a foundation-model ARN) read as an organization, so azure/gpt-4o did not match gpt-4o. Add the litellm routes, strip a foundation-model ARN like an inference profile, and pin that every provider in llm.PROVIDERS and its aliases is a route. Thread: PRRT_kwDOVBg7g86xgsT1 Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
|
Your points are answered in their threads: the stored flag of |
|
Both test gaps are fixed (8a2f952, panel size and augmenting pairing, each failing on your mutation); the points you share with the earlier review are answered in their threads. |
|
This review carries only the OpenCodeReview run marker and no claim of its own; its four findings are the inline threads, each answered there. |
|
Codecov report, no finding to act on: it reports all modified and coverable lines covered. |
|
OpenCodeReview summary, no finding of its own: its four inline findings are answered in their threads. |
vlad-perevezentsev
left a comment
vlad-perevezentsev
left a comment
There was a problem hiding this comment.
Review at 7140e6d. The fixes since e159659 are right. dokimi run stores the self-judge flag by same_model, an org/name id keeps its organization, and plain -N:M revisions and OpenAI's dashed snapshots are handled. One point is still open: an undated alias passing baseline compare --strict. My answer to your question is in that thread.
What I ran, at 7140e6d:
make ci-local --offline: 0 failed, 17 passed, 13 skipped (network/docker jobs).same_modelon the new route list:meta-llama/Xvsmyorg/Xandmeta-llama/XvsXareFalse,openrouter/google/gemma-2-9bvsgemma-2-9bandollama/qwen2.5:7bvsqwen2.5:7bareTrue.
Checked, no issues
- Stored flag:
pipeline.pyandevaluate.pynow use one rule. Under the old exact provider-and-model check,test_pipeline_independence_flag.pyfails on the cross-route pair. - Routes: only the leading segments in
_ROUTESare dropped, plus an ARN'sinference-profile/orfoundation-model/tail. A test pins everyllm.PROVIDERSname and alias as a route. - Panel tests: the augmenting-path and panel-size cases fail under a greedy pairing and with the size check removed.
- Docs and changelog: both now say the organization is part of the id and that
runstores the flag by the new rule.
Summary
Item P2-3 of #229 (Refs #229). Two rules decided "same model" and they disagreed:
After the change there is one function,
utils.same_model(a, b), and all three call sites use it:Rules chosen for each case (the table is
tests/test_same_model.py):claude-sonnet-4-5-20250929vsclaude-sonnet-4-5claude-3-5-sonnet-20240620vs…-20241022us.anthropic.xvseu.anthropic.x(bare)anthropic/xvsamazon-bedrock/x(same model, two routes)openrouter/anthropic/xvsanthropic/xm-v1:0vsm-v3:0Claude-Sonnet-4-5vsclaude-sonnet-4-5haikuvsclaude-haiku-3,deepseek-v2vsdeepseek-v3Decision points (for review):
("anthropic", "us.anthropic.claude-sonnet-5-5")as a different model was changed: it now warns.BEDROCK_MODEL_IDSalias table is now covered by the rule without a table lookup;test_a_bedrock_alias_and_its_profile_are_one_modelruns over that table.Evidence
Step 1, the case table run against both functions on
intel/main(e2147b17; 27 rows,case_table.pyin scratch): the two functions disagree on 13 rows, and both are wrong on 6 of them. Rows 11, 12, 14, 15, 17 and 18 pass a route on both sides (amazon-bedrock/us.anthropic…vsamazon-bedrock/eu.anthropic…); the oldmodel_matchesstrips a region only from the start of the string, so it says differ there. With the route on one side only (the bare-name row in the Summary table) it says same. Excerpt:Regression tests before the fix (
pytest tests/test_baseline_compare.py tests/test_model_defaults.py), the two that encode the new rule:tests/test_same_model.pyfails to collect on the old code (ImportError: cannot import name 'same_model').After the fix:
Numbers: no score formula changes. Which runs count as the same model does change:
repo comparearm pairing (_models_equivalent), the self-judge warning, andbaseline comparejudge/answer model mismatch lines and--strictrefusals. Cases that used to refuse or warn about a snapshot, region or route change now pass, and the independence warning fires for the same model on another provider.Merge Danger
Door: two-way. Reverting restores the two old rules; no data is rewritten.
Blast Radius: comparison pairing and warnings in
repo compare,baseline compareand the self-judge check. No score or stored artifact changes.Ramifications:
baseline compare --strictrefuses fewer pairs (route, region, alias changes no longer count as a model change). Any pipeline that relied on a refusal to catch a judge route change will not see it.dokimi/utils.py(get_llmhunk) and fix(cli): run, compare, arms, tasks, explore and repo exit with a message on bad input or output #238 touchesdokimi/repos/report.py(hunks abovemodel_matches). Neither hunk is edited here; the textual overlap is the removal ofmodel_matchesand its helpers in report.py and the new function in utils.py. Whichever lands second needs a rebase.changelog.d/20261028_130000_napetrov_one-model-match-rule.md(Fixed). Docs:docs/benchmarking-and-comparison.mddescribes the new rule and the panel pairing.org-a/xandorg-b/xare one model;claude-3-5-sonnet-v2@20241022(Vertex) andclaude-3-5-sonnet-20241022(direct) are not matched, because the-v2belongs to the id. A set of two snapshots against one alias is accepted by the set comparison inrepo compare(its docstring says set sizes may differ), as before."") is no longer "the same"; a malformed cell that records no resolved model now gets a mismatch line where the old code printed none.Refs #229
🤖 Generated with Claude Code