[LEADS-267] fix: metric metadata override only changes given key#192
[LEADS-267] fix: metric metadata override only changes given key#192asamal4 wants to merge 1 commit intolightspeed-core:mainfrom
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lightspeed_evaluation/core/metrics/manager.py (1)
77-85: Consider returning a copy ofbaseto avoid leaking mutable config state.When no override is present, returning
basedirectly exposes the underlyingSystemConfigmetadata dict by reference. Returning a shallow copy keeps behavior safer and consistent with the merged branch.♻️ Proposed hardening change
- return {**base, **override} if override else base + return {**base, **override} if override else dict(base) - return base if base else None + return dict(base) if base else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lightspeed_evaluation/core/metrics/manager.py` around lines 77 - 85, The current logic in manager.py returns the original `base` dict from `system_metadata.get(metric_identifier)` which can leak mutable state; update the return paths in the block that computes `base` and `level_metadata` (using `_get_level_metadata`) so that whenever `base` is returned (including the `return base if base else None` path and the `return {**base, **override} if override else base` fallback) you return a shallow copy (e.g., `base.copy()` or `dict(base)`) instead of the original object, while keeping the merged `{**base, **override}` behavior as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lightspeed_evaluation/core/metrics/manager.py`:
- Around line 77-85: The current logic in manager.py returns the original `base`
dict from `system_metadata.get(metric_identifier)` which can leak mutable state;
update the return paths in the block that computes `base` and `level_metadata`
(using `_get_level_metadata`) so that whenever `base` is returned (including the
`return base if base else None` path and the `return {**base, **override} if
override else base` fallback) you return a shallow copy (e.g., `base.copy()` or
`dict(base)`) instead of the original object, while keeping the merged `{**base,
**override}` behavior as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e7767d9-590b-4e9a-91e0-c3ed8f3d62e4
📒 Files selected for processing (2)
src/lightspeed_evaluation/core/metrics/manager.pytests/unit/core/metrics/test_manager.py
2 participants
Description
Metric metadata override from eval data should only override given key, not replace complete metadata
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Release Notes