feat: evidence-grade NIM discovery + all-modality cost-quality benchmark - #90
feat: evidence-grade NIM discovery + all-modality cost-quality benchmark#90seonghobae wants to merge 199 commits into
Conversation
…ark (#86) Optional stdlib-only benchmark harness (contextual_orchestrator/nim_benchmark.py): - Dynamic catalog discovery from the OpenAI-compatible GET /v1/models — no hard-coded inventory; deduplicated, sorted (response-order-drift immune), with machine-readable duplicate/invalid hygiene lists. - Capability probes for every contract NIM can host: chat completions, text completions, Responses API, embeddings, image understanding, video understanding, omni-style audio understanding, audio transcription, and audio speech — omni_capable derived, skipped probes always carry a machine-readable reason, bounded concurrency under one shared hard request budget. - Fair policy comparison on a locked task split: per-worker direct baselines (source of best-single-worker-in-hindsight), route_once, conduct capped at five steps, cheapest-eligible-worker; identical scorers, caps, timeouts, and token budgets across systems. - Honest cost accounting: actual cost 0 while the hosted catalog is free; hypothetical paid cost only from an explicit versioned pricing scenario, "unknown" otherwise; the two never mix. - Paired-bootstrap CIs, quality-latency and quality-hypothetical-cost Pareto frontiers, full provenance (git SHA, run id, catalog/manifest/ pricing hashes, parameters), schema-validated JSON/CSV/Markdown artifacts with a secret-leak refusal guard. - Fail closed: missing KV credential (NVIDIA_NIM_API_KEY, bootstrap env->KV only, never argv), incomplete discovery, exceeded budget, missing provenance, schema violations. - Deterministic --dry-run drives the whole pipeline against an in-process synthetic provider covering every modality class — zero network, byte-identical artifacts. - Tests: 100% statement+branch coverage of the new module, adversarial cases (malformed catalogs, duplicate ids, non-finite tokens/costs, rate limits, timeouts, order drift, secret redaction); new fuzz seam (Hypothesis + Atheris) for the catalog parser. - CI: manual + conservative monthly scheduled workflow, single-flight concurrency, hard budgets, pinned actions, 90-day artifact retention. - Docs: docs/nim_benchmark.md, architecture/tracks pointers, HELM (arXiv:2211.09110) added to docs/papers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughNVIDIA NIM 비용·품질 벤치마크 하네스를 추가했습니다. 카탈로그 탐색, modality 검증, 정책 평가, 비용·증거 검증, 결정적 dry-run, live CI 실행, artifact 생성 및 품질 테스트를 포함합니다. ChangesNIM 벤치마크 기능과 검증
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant GitHubActions
participant NIMBenchmark
participant NIMProvider
participant ArtifactStore
Operator->>GitHubActions: dry-run 또는 live 실행 요청
GitHubActions->>NIMBenchmark: 고정된 예산·manifest·provenance 전달
NIMBenchmark->>NIMProvider: 카탈로그 조회 및 capability probe
NIMProvider-->>NIMBenchmark: 모델·probe 결과 반환
NIMBenchmark->>NIMProvider: 평가 task 요청 전송
NIMProvider-->>NIMBenchmark: 응답·usage 반환
NIMBenchmark->>ArtifactStore: JSON·CSV·Markdown artifact 업로드
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
|
Exact-head maintainer audit of
Do not mark this PR ready or resolve these findings from stale checks. Re-run all required workflows and independent review on the exact repaired head. |
seonghobae
left a comment
There was a problem hiding this comment.
Blocking security finding
CRITICAL — benchmark HTTPS transport reintroduces DNS-rebinding SSRF
require_public_https_endpoint() validates one DNS answer, but build_default_transport() then calls urllib.request.urlopen(), which resolves the hostname again. The validated_hosts cache widens the gap by skipping validation on later calls. A provider hostname can therefore pass public-address validation and later connect to loopback, private, RFC 6598, link-local, or otherwise non-global infrastructure. Redirects are also delegated to urllib's default handler, so credentials may be forwarded to an unvalidated destination.
This is the same TOCTOU class repaired by PR #76. Keep this PR Draft until #76 is in main, then reuse or generalize its DNS-pinned transport so the socket dials only validation-time public IPs while preserving the original hostname for HTTP authority, TLS SNI, and certificate verification. Reject redirects, bypass environment proxies, require is_global, clear stale pins before each validation, and deterministically close failed sockets/responses.
Required regression evidence:
- no transport-time DNS re-resolution;
- public IPv4/IPv6 pinning and hostname/SNI preservation;
- RFC 6598/private/loopback/link-local/multicast/reserved/unspecified rejection;
- redirect rejection with no authorization propagation;
- approved-IP fallback and all-address failure;
- exact-head statement/branch coverage and full repository checks.
Do not merge based on the current local 100% claim; it does not cover this security invariant.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review — still blocked
The latest head addresses the three audit themes, but the implementation is not yet mergeable.
1. HIGH — production security behavior is coupled to a test monkeypatch
nim_benchmark_hardening._build_secure_transport() deliberately falls back to urllib.request.urlopen() whenever that global has been replaced. This keeps the old offline tests green by changing production behavior instead of replacing the obsolete tests. It reintroduces the hostname-resolving, proxy-aware, redirect-capable path that this repair is supposed to eliminate, and requires a nosemgrep waiver on the exact sink.
Remove the compatibility branch entirely. Rewrite the transport tests to inject the pinned connection/resolver seam and prove the real direct transport. Do not let test instrumentation select a less-secure production path.
2. HIGH — the new 531-line hardening module has no direct regression suite
The current tests/test_nim_benchmark.py still asserts the old urlopen behavior and contains no evidence for EqualBudgetModelClient, expiry enforcement, actual-cost evidence validation, configured/observed budget fields, or the installed wrappers. A 100% repository claim cannot be accepted while the newly shipped module and its branches are not directly exercised.
Add behavior tests that fail without each contract, then prove exact-head statement and branch coverage at 100% for both nim_benchmark.py and nim_benchmark_hardening.py, plus 100% public docstrings.
3. HIGH — cited cost source does not support the recorded claim
ACTUAL_COST_EVIDENCE names https://docs.nvidia.com/nim/large-language-models/latest/faq.html, but the current NVIDIA FAQ location is different and the available FAQ content does not establish that the API Catalog hosted endpoint used by this run is free to the caller. Current build.nvidia.com model pages do label prototype endpoints as free, while NIM offering/licensing documentation distinguishes free exploratory offerings from NVIDIA AI Enterprise production support. Record the exact reviewed page/version or immutable evidence artifact that supports the hosted endpoint claim; do not cite a generic or moved FAQ as proof.
4. MEDIUM — optional adapter boundary is lost
contextual_orchestrator.__init__ now imports the entire benchmark and monkeypatch installer for every package import. Keep the NIM evaluator optional: integrate the fix directly into its module, or install it only from the benchmark entrypoint. Runtime gateway users should not import benchmark-only code or mutate benchmark globals as a side effect of import contextual_orchestrator.
5. MEDIUM — temporary privileged workflow remains in the PR tree
.github/workflows/temporary-nim-transport-fix.yml has top-level contents: write, persists checkout credentials, and triggers on every branch push. It must be removed from the final tree. Any one-shot repair must use read-only defaults, isolate credentials to the publication step, validate exact changed paths, and disappear before review.
Keep the PR Draft. Replace the compatibility installer with direct, test-first implementation; remove the temporary workflow; update the PR body to the actual head; then rerun all exact-head checks and independent review.
|
Exact-head maintainer follow-up for the new hardening commits:
Do not resolve the security thread or mark the PR ready until these are implemented and verified on the exact repaired head. |
Purpose
Implement issue #86 as a provider-neutral, evidence-grade NVIDIA NIM discovery and benchmark harness while preserving standalone operation and the modular CWL MSA boundary.
Exact integration identity
a9a37e28c1790be43cbdbd472d8e1017762b71f5fix/atheris-interpreter-lock2df63e080737d5aa29d40871af99aa13411c3d7607fe73b1e9f8c3b3c564a9a3d4fdc47dfb36d2c1The base branch is independently maintained and may move. Before every mutation and acceptance decision, GitHub PR metadata is authoritative. Any check, status, review, approval, or synthetic merge associated with another head/base pair is historical only.
Current-head verification state
Repository workflows have materialized for exact head
a9a37e28c1790be43cbdbd472d8e1017762b71f5but are not yet passing evidence:31174602772: queued;31174602823: queued;31174602869: queued;Queued evidence is not success. Security Scan, SAST Semgrep, trusted 100% statement/branch/docstring/package evidence, OpenCode, Noema, Strix, release acceptance, and qualifying independent non-author approval are absent for this exact head. One valid transactional-publication review thread remains intentionally unresolved until its current-head evidence requirement is actually satisfied.
The previous stacked-base merge conflict is repaired without weakening either side: repository-local workflows retain #96's arbitrary-base exact-contributor-head checkout contract, the exact-head regression matches the current base, and
CHANGELOG.mdpreserves current provider-response and stacked-workflow evidence while moving the NIM feature record to a conflict-free Added section.Implemented benchmark scope
GET /v1/modelscatalog without a hard-coded authoritative inventory;route_once, boundedconduct, and an optional reviewed cheapest-worker scenario under common total token and call envelopes;unknown;Strict scoring validity boundary
A test-first buyer-visible slice repairs construct-irrelevant score inflation in the locked task set.
337356117ede1b69ba1b47282038bfa0fcc9c360required complete-answer scorers that did not yet exist.exact_number_matchversion2accepts only one complete finite decimal response; prose, negation, units, multiple values,NaN, and infinities do not earn credit.exact_text_matchversion1compares the complete NFC-normalized, whitespace-normalized, case-folded response against explicit declared alternatives; substrings and undeclared aliases do not match.import contextual_orchestratordoes not import the benchmark, CSV adapter, or strict-scoring adapter;An isolated networkless harness exercised all 142 strict-scoring production statements and 54 branches and passed; this diagnostic validates the bounded module logic but is not accepted as repository CI or merge evidence. Authoritative doctoring is in
docs/doctoring/nim-benchmark-strict-answer-scoring.md.Provider and credential security boundary
NVIDIA_NIM_API_KEY; only the bounded live benchmark step receives the GitHub Secret;COPILOT_GITHUB_TOKENpath is introduced;Central dependencies — read-only
Central
.github#759is closed without merge at exact headdcb87ea3421ec87ce6acc62b04beef56758aa33e; none of its predecessor evidence transfers.The active protected-main prerequisites are owned exclusively by the central maintenance path:
.github#812: Draft at exact head2137012a2bf57b19a36f20ff6dcaac3e3e3b8cba, repairing attempt-scoped coverage artifact recovery control flow;.github#816: Draft at exact head71f74df36bbe322b1022d6eb3fa5aa7ac53d0643, repairing LLVM 19 propagation and revalidation across the isolated Rust coverage runtime.This repository neither writes those branches nor dispatches repair agents there. PR #96 must remain Draft until the required central changes reach protected
.githubmain, then regenerate fresh exact-head central coverage and review evidence and merge first. This PR must subsequently retarget to integratedmainand rerun every gate on the exact resulting head/base pair.Evidence contracts
Permanent regressions cover complete-plan admission, one-request-short zero-partial egress, deterministic bounded concurrency, valid media fixtures, DNS rebinding and non-global egress, redirects, proxy and credential isolation, bounded provider bodies, equal token/call budgets, reviewed pricing expiry, strict scoring, complete assignment evidence, Pareto exclusion labels, schema completeness, transactional publication, rollback/crash recovery, package import isolation, exact contributor-head workflow checkout, fuzz instrumentation, and buyer-readable evidence-status semantics.
Stack order and merge acceptance
.github#812and.github#816must reach protected centralmainunder their own maintenance path;main, reconcile once if its branch or base moved, and rerun all exact-head gates;Queued, pending, skipped-required, cancelled, absent, stale-head, predecessor-head, failed, status-only, author-only, or synthetic-merge evidence is never success.
Refs #86.
Depends on #96 and the protected-main central repairs tracked by ContextualWisdomLab/.github#812 and #816.