feat(nim): add transactional benchmark evidence boundary - #890
Conversation
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughNVIDIA NIM 벤치마크 증거 경계를 추가했다. 매니페스트와 provenance를 엄격히 검증하고, 네 개의 artifact를 잠금 기반으로 원자적으로 게시한다. 게시 실패와 충돌 잔여물을 복구하며, 계약 테스트와 문서로 동작을 정의한다. ChangesNIM 증거 경계
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The change publishes benchmark evidence transactionally, but newly created evidence directories may be world-writable and therefore tamperable by other local users, weakening artifact integrity; merge should wait for an explicit restrictive permission mode. Two failure-path tests also need platform-safe path comparisons. Sequence Diagram(s)sequenceDiagram
participant Caller as 호출자
participant Publisher as publish_artifact_set
participant Lock as 게시 잠금
participant Filesystem as 파일 시스템
Caller->>Publisher: artifact와 output_directory 전달
Publisher->>Publisher: payload 및 provenance 검증
Publisher->>Lock: sibling lock 획득
Lock->>Filesystem: staging 기록 및 기존 final backup
Filesystem->>Publisher: staging을 final로 원자적 교체
Publisher->>Lock: 잠금 해제
Publisher-->>Caller: 완료 또는 NimEvidenceError
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 36 functions across 2 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| try: | ||
| os.replace(staging, final) | ||
| except BaseException: | ||
| if backup is not None and backup.exists(): | ||
| os.replace(backup, final) | ||
| raise |
There was a problem hiding this comment.
📝 Info: Failed restore leaves target missing until next publish
If os.replace(staging, final) fails and the restore os.replace(backup, final) also fails, the finally block removes staging and final is left absent, with old data only in the .name.backup-* directory. The next publish's _recover_publication restores it, so the loss is transient, not permanent.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ories - Compare staging cleanup locks by resolving parent paths
| for name, payload in artifacts.items(): | ||
| (staging / name).write_bytes(payload) | ||
| if final_mode is not None: | ||
| staging.chmod(final_mode) | ||
| if final.exists(): | ||
| backup = final.parent / f".{final.name}.backup-{uuid.uuid4().hex}" | ||
| os.replace(final, backup) | ||
| try: | ||
| os.replace(staging, final) |
There was a problem hiding this comment.
📝 Info: No fsync before atomic rename commit
Staging files are written and committed with os.replace (contextual_orchestrator/nim_evidence.py:209-217) without fsyncing the files or the parent directory. After power loss right after the rename, POSIX does not guarantee the contents or the rename are durable. The crash-recovery guarantee holds only against process crashes, not power loss.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Outcome
Adds the first independently reviewable slice of #86: a fail-closed evidence boundary for optional NVIDIA NIM benchmarks. It validates versioned task/scorer identities and publishes exactly JSON, CSV, Markdown, and provenance as one same-filesystem directory transaction.
Boundaries
unknownfor an absent reviewed pricing scenario instead of converting it to zero/free.Verification on exact head
3fef47b74700f1fdc2401e39491e0077f3423ec4uvx ruff check contextual_orchestrator/nim_evidence.py tests/test_nim_evidence.pyuv run --with pytest --with pytest-cov pytest -q tests/test_nim_evidence.py --cov=contextual_orchestrator.nim_evidence --cov-branch --cov-report=term-missing— 13 passed; 99 statements and 50 branches at 100%uv run --extra test --with pytest pytest -q— 2366 passed in 740.60sRemaining #86 work
Dynamic live catalog/capability probing, Rust-owned paired uncertainty/Pareto analysis, full benchmark policy comparison, and protected-main operational acceptance remain open under #86.
Closes no issue; contributes to #86.
Summary by CodeRabbit
새 기능
문서