implement registry functions and fix linting (#12) - #49
Conversation
Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
WalkthroughAdds thread-safe mutation APIs to the metrics registry: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/agentunit/metrics/registry.py (1)
51-56: Consider deduplicating lookup logic.The lookup pattern (get from DEFAULT_METRICS, check None, raise KeyError) is duplicated between
get_metricandresolve_metrics(lines 36-39). While the duplication is minimal, you could reduce it by having one function call the other.🔎 Example refactor to reduce duplication
def get_metric(name: str) -> Metric: - metric = DEFAULT_METRICS.get(name) - if metric is None: - msg = f"Unknown metric '{name}'" - raise KeyError(msg) - return metric + return resolve_metrics([name])[0]tests/test_metrics_registry.py (1)
22-29: Consider adding test coverage for successful metric registration.While this test correctly validates duplicate detection, there's no test for the successful registration path. Consider adding a test that registers a new metric and then retrieves it, with proper cleanup to avoid polluting the global state for other tests.
🔎 Example test with cleanup
def test_register_metric_success(): from agentunit.metrics.registry import DEFAULT_METRICS from agentunit.metrics.builtin import ToolSuccessMetric # Register a new metric with a unique name test_metric = ToolSuccessMetric() register_metric("test_custom_metric", test_metric) try: # Verify it was registered retrieved = get_metric("test_custom_metric") assert retrieved is test_metric finally: # Cleanup to avoid polluting other tests DEFAULT_METRICS.pop("test_custom_metric", None)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/agentunit/metrics/registry.pytests/test_metrics_registry.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_metrics_registry.py (2)
src/agentunit/metrics/registry.py (3)
get_metric(51-56)register_metric(44-48)resolve_metrics(31-41)src/agentunit/metrics/builtin.py (2)
FaithfulnessMetric(37-57)ToolSuccessMetric(60-70)
src/agentunit/metrics/registry.py (2)
src/agentunit/datasets/base.py (1)
name(38-39)src/agentunit/metrics/base.py (1)
Metric(23-26)
🔇 Additional comments (2)
tests/test_metrics_registry.py (2)
12-18: LGTM!The test correctly validates error handling for unknown metrics and checks the error message.
32-39: LGTM!The test correctly validates successful metric retrieval and verifies the returned type.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/agentunit/metrics/registry.py (1)
31-31: Consider documenting the concurrency model.The lock correctly guards write operations in
register_metric, but read operations (resolve_metrics,get_metric) accessDEFAULT_METRICSwithout synchronization. This is acceptable if metric registration occurs only during application startup before concurrent access begins.Consider adding a module-level docstring or comment documenting the expected usage pattern to prevent future misuse.
📝 Example documentation
"""Metric registry mapping string names to implementations. Thread safety: Metric registration via register_metric() is thread-safe. For best results, register all metrics during application initialization before concurrent access begins. """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/agentunit/metrics/registry.pytests/test_metrics_registry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_metrics_registry.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/agentunit/metrics/registry.py (1)
src/agentunit/metrics/base.py (1)
Metric(23-26)
🔇 Additional comments (3)
src/agentunit/metrics/registry.py (3)
5-5: LGTM: Import correctly placed.The
threadingimport is properly positioned according to PEP8 conventions (stdlib imports in alphabetical order).
47-52: LGTM: Thread-safe registration with proper validation.The implementation correctly:
- Guards the check-then-set operation with
_registry_lockto prevent race conditions- Validates for duplicate registrations before adding
- Provides clear error messages
55-58: LGTM: Clean delegation pattern.The function correctly delegates to
resolve_metricsto avoid duplicating error-handling logic. The comment clearly explains the rationale for this design choice. The KeyError raised byresolve_metricswill propagate naturally when the metric is not found.
|
All requested changes have been implemented and verified: Thread Safety: Added threading.Lock to register_metric to guard global state mutation. Deduplication: Refactored get_metric to use resolve_metrics, ensuring consistent error messages across the registry. Test Robustness: Updated test_resolve_default_metrics with dynamic length checks and added test_register_metric_success with proper state cleanup. CI Status: Local poetry build and ruff format are successful, and all GitHub CI checks are now Green. The PR is ready for final review and merging. Thank you so much @aviralgarg05 for the guidance 🙏🙏 |
3 participants
Description
This PR addresses the missing functionality in the metrics registry by implementing the registration and retrieval logic. This allows for dynamic expansion of the metrics suite and ensures better reliability through comprehensive unit tests.
Fixes #12
Type of Change
Please delete options that are not relevant:
Changes Made
Implemented register_metric() in src/agentunit/metrics/registry.py to allow adding new metrics at runtime.
Implemented get_metric() in src/agentunit/metrics/registry.py to safely retrieve metrics by their string names.
Created tests/test_metrics_registry.py with 4 new test cases.
Ensured all imports follow PEP8/Ruff guidelines by placing them at the top of the file.
Testing
pytest)Test Configuration
Test Results
============================= test session starts ==============================
platform linux -- Python 3.12.1, pytest-8.4.2, pluggy-1.6.0
rootdir: /workspaces/agentunit
configfile: pyproject.toml
plugins: agentunit-0.7.0, cov-5.0.0, anyio-4.11.0
collected 4 items
tests/test_metrics_registry.py .... [100%]
============================== 4 passed in 0.02s ===============================
Code Quality
Documentation
Dependencies
Performance Impact
Additional Context
Add any other context about the PR here:
Checklist
Reviewer Notes
Please pay special attention to:
Post-Merge Tasks
Tasks to complete after merging (if any):
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.