RPOPC-1317: Extract STREAMS benchmark version from CSV metadata#49
Conversation
- Add test for extracting version from CSV comment - Add test for whitespace variations - Add test for fallback to wrapper version when missing - Add test for using first occurrence when multiple versions - Add test for different version number formats Part of RPOPC-1317. Tests currently fail (RED phase).
- Add _benchmark_version instance variable to store extracted version - Extract version from '# streams_version_# X.Y' CSV comment in _parse_streams_csv() - Use first occurrence if multiple version comments present - Override build_test_info() to use benchmark version for test.version - Fall back to wrapper version when benchmark version not found Implements RPOPC-1317. All tests passing (GREEN phase). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 53 minutes and 35 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesSTREAMS benchmark version extraction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
grdumas
left a comment
grdumas
left a comment
There was a problem hiding this comment.
PR Review: RPOPC-1317: Extract STREAMS benchmark version from CSV metadata
Summary
This PR successfully addresses the version conflation issue for the STREAMS benchmark by correctly parsing its version from the CSV metadata comments and utilizing the newly established build_test_info() override pattern.
Critical Issues (MUST FIX)
None found.
Security Delta
None found. No security-sensitive code was removed or weakened.
Major Issues (SHOULD FIX)
None found.
Minor Issues (NICE TO HAVE)
None found.
Nitpicks (OPTIONAL)
None found.
Positive Notes
- Pattern Adoption: Excellent use of the
build_test_info()override pattern established in the base processor. The fallback logicself._benchmark_version or base_info.versionis clean and defensive. - Edge Case Handling: The regex
r'streams_version_#\s+(\S+)'correctly handles arbitrary whitespace variations, and the conditionif self._benchmark_version is None:safely ensures that only the first version comment in the file is captured. - Testing: The 5 new unit tests are comprehensive, covering variations in whitespace, missing versions, multiple version strings, and different version formats.
Overall Assessment
- Status: APPROVE
- Reasoning: The code cleanly extracts the correct benchmark version without impacting the existing wrapper version assignment. It handles the parsing robustly and includes a strong suite of tests.
- Next Steps: Ready to merge.
Reviewed by: Gemini Pro via automated code review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_streams_version_extraction.py (1)
23-109: ⚡ Quick winAdd a regression test for processor reuse across multiple parses.
Current tests don’t exercise calling
parse_runs()twice on the sameStreamsProcessorinstance (first CSV hasstreams_version_#, second CSV omits it). That scenario should asserttest.versionfalls back correctly on the second parse and does not retain stale benchmark version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_streams_version_extraction.py` around lines 23 - 109, Add a new regression test function to verify that the StreamsProcessor correctly handles multiple sequential parse_runs() calls without retaining stale state. Create a test that instantiates a StreamsProcessor once, calls parse_runs with a CSV containing a streams_version_# comment (e.g., "5.10"), then calls parse_runs again with a different CSV that omits the version comment, and verifies that build_test_info() returns the fallback wrapper version (not the stale "5.10" from the first parse). This ensures that the processor resets its benchmark version appropriately on subsequent parses when the comment is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chronicler/processors/streams_processor.py`:
- Around line 223-227: The `_benchmark_version` attribute in the
`StreamsProcessor` class is captured only once per processor instance and never
reset before a new parse, causing stale benchmark version metadata to persist
across multiple input parses when the same processor instance is reused. Reset
`_benchmark_version` to None at the beginning of the parse method (or whichever
method initiates a new parse operation) to ensure each parse starts with a clean
state and captures the correct version for the current input.
---
Nitpick comments:
In `@tests/test_streams_version_extraction.py`:
- Around line 23-109: Add a new regression test function to verify that the
StreamsProcessor correctly handles multiple sequential parse_runs() calls
without retaining stale state. Create a test that instantiates a
StreamsProcessor once, calls parse_runs with a CSV containing a
streams_version_# comment (e.g., "5.10"), then calls parse_runs again with a
different CSV that omits the version comment, and verifies that
build_test_info() returns the fallback wrapper version (not the stale "5.10"
from the first parse). This ensures that the processor resets its benchmark
version appropriately on subsequent parses when the comment is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 8a31cf16-7453-44eb-902e-ebee28b4d7dd
📒 Files selected for processing (2)
src/chronicler/processors/streams_processor.pytests/test_streams_version_extraction.py
Add test to verify _benchmark_version doesn't leak between parse_runs() calls when reusing the same processor instance. Currently fails (RED phase) - demonstrates the bug where second parse incorrectly retains first parse's version. Addresses review feedback on PR #49.
Reset self._benchmark_version to None at the start of parse_runs() to prevent state leakage when the same processor instance is reused across multiple parses. Without this fix, if a processor parsed CSV1 (with version) then CSV2 (without version), CSV2 would incorrectly inherit CSV1's benchmark version instead of falling back to wrapper version. Addresses review feedback on PR #49. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
PR Update: Addressed Review FeedbackWhat was done
What was not doneNone - all requested changes were implemented. Why this approachThe state leakage bug was subtle because all existing tests created fresh Resetting
VerificationAll 274 tests pass (6 version extraction tests + 7 other STREAMS tests + 261 other tests). Before fix: After fix: The PR is now ready for re-review. Responded by: Claude Sonnet 4.5 via automated workflow |
grdumas
left a comment
grdumas
left a comment
There was a problem hiding this comment.
PR Review: RPOPC-1317: Extract STREAMS benchmark version from CSV metadata
Summary
This update cleanly addresses the potential issue of state leakage when a single StreamsProcessor instance processes multiple distinct test runs sequentially.
Critical Issues (MUST FIX)
None found.
Major Issues (SHOULD FIX)
None found.
Minor Issues (NICE TO HAVE)
None found.
Nitpicks (OPTIONAL)
None found.
Positive Notes
- State Management: Resetting
self._benchmark_version = Noneat the top ofparse_runs()is a great catch and an excellent practice for defensive programming, ensuring no stale data leaks between processing tasks. - Testing: The new test,
test_streams_version_resets_between_parses, flawlessly proves the regression is avoided. Testing stateful behaviors like this is critical for long-running batch systems.
Overall Assessment
- Status: APPROVE
- Reasoning: The core implementation remains robust, and the state-leakage fix ensures safety during batch processing. Test coverage remains excellent and all 274 tests pass.
- Next Steps: Ready to merge.
Reviewed by: Gemini Pro via automated code review
1 participant
Summary
Fix StreamsProcessor to extract the STREAMS benchmark version from CSV metadata comments instead of incorrectly using the wrapper version.
Problem
StreamsProcessor was using the wrapper version (e.g., "v2.8") for
test.versioninstead of the actual STREAMS benchmark version (e.g., "5.10") found in CSV metadata.Acceptance Criteria
_parse_streams_csv()streams_version_#followed by version numberself._benchmark_versionbuild_test_info()to use the extracted benchmark version instead of wrapper versiontest.versioncontains the benchmark version (e.g., "5.10") not the wrapper version (e.g., "v2.8")Changes
_benchmark_versioninstance variable to StreamsProcessor# streams_version_# X.Ycomments in_parse_streams_csv()build_test_info()to prioritize benchmark version over wrapper versionTesting
Related