-
Notifications
You must be signed in to change notification settings - Fork 15
Fix miscellaneous type hints #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix miscellaneous type hints #45
Conversation
Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
WalkthroughReplaces LangSmith-specific flows with AgentOps tracing and session handling in the adapter; makes run_scenario async; broadens factory method option typings to Any; introduces a DatasetRow TypedDict; adds defensive metadata access and an explicit baseline_stats type annotation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Adapter as AgentOpsAdapter
participant Runner as ScenarioRunner
participant AgentOps as AgentOpsClient
Client->>Adapter: run_scenario(scenario)
Note right of Adapter: run_scenario is async
Adapter->>AgentOps: start_trace(trace_metadata)
AgentOps-->>Adapter: trace_id
Adapter->>Runner: execute scenario (trace_id)
Runner-->>Adapter: events / result
Adapter->>AgentOps: update_trace_metadata(trace_id, metrics/result)
alt success
Adapter->>AgentOps: end_trace(trace_id, status="completed")
else failure
Adapter->>AgentOps: end_trace(trace_id, status="failed", details)
end
Adapter-->>Client: ScenarioResult (scenario_run_id from trace_id)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agentunit/adapters/agentops_adapter.py (1)
73-87: Remove inconsistent and outdated docstring block.Lines 73-87 contain a duplicate/outdated docstring that references "LangSmith" instead of "AgentOps" and has incorrect parameter descriptions (e.g., "Langsmith project ID" on line 78). This conflicts with the actual AgentOps docstring above (lines 56-65).
🔎 Proposed fix
- """ - Initialize LangSmith adapter. - - Args: - api_key: LangSmith API key - project_id: Langsmith project ID - endpoint: Optional custom LangSmith endpoint - enable_tracing: Whether to enable automatic tracing - enable_feedback: Whether to collect feedback data - """ - self.api_key = api_key - self.project_id = project_id - self.default_tags = default_tags or [] - self.auto_start_session = auto_start_session -
🧹 Nitpick comments (2)
src/agentunit/adapters/agentops_adapter.py (2)
70-71: Type annotation added for new attributes.The
self.client: Anyannotation provides some type information, thoughAnysacrifices type safety. Consider using a protocol or the actual agentops client type when available for better type checking.
103-103: Document the import suppression or address the underlying issue.The
# type: ignore[import-not-found]comment suppresses mypy's missing import error. This aligns with the PR description noting that ragas import issues remain. Consider documenting why this is acceptable (e.g., optional dependency) in a comment.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/agentunit/adapters/agentops_adapter.py(13 hunks)src/agentunit/core/scenario.py(12 hunks)src/agentunit/datasets/builtins.py(4 hunks)src/agentunit/metrics/builtin.py(2 hunks)src/agentunit/production/integrations.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/agentunit/adapters/agentops_adapter.py (4)
src/agentunit/adapters/langsmith_adapter.py (2)
platform(104-106)run_scenario(355-516)src/agentunit/production/integrations.py (1)
MonitoringPlatform(33-41)src/agentunit/core/scenario.py (1)
Scenario(23-278)src/agentunit/reporting/results.py (1)
ScenarioResult(32-66)
src/agentunit/datasets/builtins.py (1)
src/agentunit/datasets/base.py (1)
DatasetCase(19-27)
src/agentunit/core/scenario.py (2)
src/agentunit/datasets/base.py (1)
name(38-39)tests/test_framework_adapters.py (1)
flow(105-107)
🔇 Additional comments (11)
src/agentunit/production/integrations.py (1)
100-100: LGTM! Type annotation correctly describes the baseline stats structure.The explicit type annotation accurately reflects the nested dictionary structure used for categorizing baseline statistics by metric category, metric name, and statistical measures.
src/agentunit/metrics/builtin.py (2)
146-148: LGTM! Defensive metadata access prevents AttributeError.The change from direct
trace.metadataaccess togetattr(trace, "metadata", {})safely handles cases where the trace object may not have a metadata attribute, falling back to an empty dict.
172-174: LGTM! Consistent defensive pattern applied.The same safe metadata access pattern is correctly applied here, maintaining consistency with CostMetric above.
src/agentunit/datasets/builtins.py (4)
16-24: LGTM! DatasetRow TypedDict correctly structures dataset rows.The TypedDict definition properly mirrors the DatasetCase dataclass structure from base.py, with appropriate use of
NotRequiredfor the optional metadata field. This provides strong type checking for dataset row dictionaries.
27-27: LGTM! Explicit type annotation improves type safety.The type annotation
list[DatasetRow]provides clear type information for the dataset, enabling better IDE support and type checking.
44-44: LGTM! Consistent type annotation applied.The same
list[DatasetRow]annotation is consistently applied to this dataset.
64-64: LGTM! Function signature properly typed.The updated signature with
list[DatasetRow]parameter type andIterable[DatasetCase]return type provides clear type contracts for the loader function.src/agentunit/core/scenario.py (2)
98-98: **LGTM! UsingAnyfor kwargs is more appropriate thanobject.The change from
**options: objectto**options: Anyis the correct approach for variadic keyword arguments, providing better flexibility while maintaining type safety at call sites.Also applies to: 116-116, 131-131, 146-146, 161-161, 176-176, 191-191, 207-207, 222-222, 242-242
71-71: Verify the necessity of the additional None check.The change from
getattr(flow, "__name__", "openai-agents-scenario")togetattr(flow, "__name__", None) or "openai-agents-scenario"adds protection against falsy__name__values (like empty strings). However, this differs from the pattern used in other factory methods (lines 89, 107, 122, etc.).Is there a specific case where
flow.__name__could be an empty string or other falsy value? If not, consider keeping consistency with other factory methods for maintainability.#!/bin/bash # Check if other factory methods in the same file use similar patterns rg -n "getattr.*__name__" src/agentunit/core/scenario.pysrc/agentunit/adapters/agentops_adapter.py (2)
355-355: LGTM! Explicit type annotation improves clarity.The
agent_counts: dict[str, int]annotation clearly documents the structure of the dictionary being built.
385-385: AgentOpsAdapter.run_scenario is async, but inconsistently implemented across adapters.The
run_scenariomethod in AgentOpsAdapter is now async, while LangSmithAdapter and WandbAdapter retain sync implementations. This creates API inconsistency: the abstract base class declares it as async, but not all adapters comply. Additionally, the runner's main execution path callsadapter.execute()rather thanadapter.run_scenario(), so this breaking change only affects direct callers of the method. Consider either making all adapters async or clarifying whetherrun_scenariois the intended public interface (if not, it could be internal).
| if isinstance(interaction.from_agent, list): | ||
| unique_agents.update(interaction.from_agent) | ||
| else: | ||
| unique_agents.add(interaction.from_agent) | ||
| if interaction.to_agent: | ||
| unique_agents.add(interaction.to_agent) | ||
| if isinstance(interaction.to_agent, list): | ||
| unique_agents.update(interaction.to_agent) | ||
| else: | ||
| unique_agents.add(interaction.to_agent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the agentops_adapter.py file
find . -type f -name "agentops_adapter.py" | head -20
# Check the file structure
fd "agentunit" --type d | head -10Repository: aviralgarg05/agentunit
Length of output: 125
🏁 Script executed:
#!/bin/bash
# First, let's check if the file exists and get basic info
if [ -f "src/agentunit/adapters/agentops_adapter.py" ]; then
echo "File found. Checking lines 240-260 for AgentInteraction definition..."
sed -n '240,260p' src/agentunit/adapters/agentops_adapter.py
echo ""
echo "Checking lines 342-350 for the code in review..."
sed -n '342,350p' src/agentunit/adapters/agentops_adapter.py
else
echo "File not found at expected location. Searching for it..."
find . -name "agentops_adapter.py" -type f 2>/dev/null
fi
# Also search for AgentInteraction class definition
echo ""
echo "Searching for AgentInteraction class definition..."
rg "class AgentInteraction" --type py -A 10 2>/dev/null || echo "Pattern not found with rg"Repository: aviralgarg05/agentunit
Length of output: 2159
Remove unnecessary list handling for from_agent.
The to_agent field is properly typed as AgentID | list[AgentID] to support broadcast messaging, so the list checks for that field are correct. However, from_agent is typed only as AgentID (string), not as a union type. Remove the isinstance check for from_agent being a list, or update its type hint to AgentID | list[AgentID] if list support is intentional.
🤖 Prompt for AI Agents
In src/agentunit/adapters/agentops_adapter.py around lines 342 to 350, the code
treats interaction.from_agent as possibly being a list but its type is only
AgentID (string); remove the unnecessary isinstance(interaction.from_agent,
list) branch and simply call unique_agents.add(interaction.from_agent), unless
you intentionally want to support broadcasting from multiple agents—in that case
update the type hint for from_agent to AgentID | list[AgentID] and keep the
existing list handling; ensure any callers and type annotations are updated to
match the chosen approach.
aviralgarg05
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the issues
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/agentunit/adapters/agentops_adapter.py (8)
73-86: Remove duplicate LangSmith code block.Lines 73-86 contain a duplicate docstring and attribute assignments copied from the LangSmith adapter. The docstring incorrectly states "Initialize LangSmith adapter" and "Langsmith project ID", and the attribute assignments (api_key, project_id, default_tags, auto_start_session) redundantly repeat lines 66-69. This dead code reduces maintainability and creates confusion.
🔎 Proposed fix
- """ - Initialize LangSmith adapter. - - Args: - api_key: LangSmith API key - project_id: Langsmith project ID - endpoint: Optional custom LangSmith endpoint - enable_tracing: Whether to enable automatic tracing - enable_feedback: Whether to collect feedback data - """ - self.api_key = api_key - self.project_id = project_id - self.default_tags = default_tags or [] - self.auto_start_session = auto_start_session - # Initialize AgentOps client self._initialize_agentops()
380-388: Update docstring and log message to reference AgentOps instead of LangSmith.The docstring at lines 380-386 states "Run a scenario with LangSmith integration" and the log message at line 389 references "LangSmith", but this is the AgentOps adapter. Update all references to reflect the correct integration.
🔎 Proposed fix
async def run_scenario(self, scenario: Scenario) -> ScenarioResult: """ - Run a scenario with LangSmith integration. + Run a scenario with AgentOps integration. Args: scenario: Scenario to execute Returns: - ScenarioResult: Execution results with LangSmith trace data + ScenarioResult: Execution results with AgentOps trace data """ - logger.info(f"Running scenario with LangSmith: {scenario.name}") + logger.info(f"Running scenario with AgentOps: {scenario.name}") - # Start LangSmith run for the scenario + # Start AgentOps run for the scenario scenario_run_id = None
484-496: Update remaining LangSmith references to AgentOps.Lines 484, 524, and associated code comments still reference "LangSmith run" when they should reference AgentOps traces. Update these comments for consistency.
🔎 Proposed fix
- # Update LangSmith run with results + # Update AgentOps trace with results if scenario_run_id and self.enable_tracing: try: self.agentops.update_trace_metadata(Apply similar changes to line 524.
Also applies to: 524-534
537-548: Updatecollect_metricsdocstring to reference AgentOps.The docstring at lines 539-547 states "Collect production metrics from LangSmith" but this is the AgentOps adapter. Update the docstring to reflect the correct integration platform.
🔎 Proposed fix
def collect_metrics(self, scenario: Any, result: Any, **kwargs) -> ProductionMetrics: """ - Collect production metrics from LangSmith. + Collect production metrics from AgentOps. Args: scenario: The scenario being evaluated
612-677: Updateestablish_baselinedocstring and log messages to reference AgentOps.The docstring at line 616 states "Establish baseline metrics from historical LangSmith data" and log messages reference "LangSmith baseline" and "LangSmith" errors. Update all references to reflect AgentOps integration.
🔎 Proposed fix
def establish_baseline( self, historical_data: list[dict[str, Any]], metrics: list[str], **kwargs ) -> BaselineMetrics: """ - Establish baseline metrics from historical LangSmith data. + Establish baseline metrics from historical AgentOps data. Args: historical_data: Historical data for baseline calculationApply similar updates to log messages at lines 642, 670, etc.
679-697: Update helper method docstring to reference AgentOps.Line 680 docstring states "Extract metrics from LangSmith runs" but should reference AgentOps for consistency with the adapter's purpose.
🔎 Proposed fix
def _extract_run_metrics(self, runs): - """Extract metrics from LangSmith runs.""" + """Extract metrics from AgentOps runs.""" durations = []
713-747: Updatecreate_evaluation_datasetdocstring and log messages to reference AgentOps.The docstring at line 717 and log messages at lines 742 and 746 reference "LangSmith" when they should reference AgentOps.
🔎 Proposed fix
def create_evaluation_dataset( self, name: str, examples: list[dict[str, Any]], description: str | None = None ) -> str: """ - Create an evaluation dataset in LangSmith. + Create an evaluation dataset in AgentOps. Args: name: Dataset nameApply similar updates to log messages.
749-776: Critical:run_evaluationuses LangSmith SDK instead of AgentOps SDK.The
run_evaluationmethod at lines 749-776 imports and callslangsmith.evaluation.evaluate, but in an AgentOps adapter whereself.clientis initialized as the AgentOps SDK (line 71:self.client = agentops). This will cause a runtime failure. Additionally, the current AgentOps Python SDK does not provide evaluation dataset or evaluation-run APIs (marked as "forthcoming" in their roadmap). Either remove this method or completely rewrite it using AgentOps APIs once they become available. Also fix the duplicate LangSmith docstring in__init__(lines 73-82).
🧹 Nitpick comments (1)
src/agentunit/adapters/agentops_adapter.py (1)
72-72: Consider makingplatforma property for consistency with other adapters.Line 72 assigns
self.platformdirectly as an instance attribute. However, other adapters (LangSmithAdapter and WandbAdapter) implementplatformas a@propertymethod returningMonitoringPlatform. For consistency across adapters, consider refactoring this to match the pattern:🔎 Proposed refactor
Remove line 72 and add a property method after the
__init__method:- self.platform = MonitoringPlatform.AGENTOPSThen add after the
__init__method (around line 98):@property def platform(self) -> MonitoringPlatform: """Return the monitoring platform type.""" return MonitoringPlatform.AGENT_OPSNote: Use
MonitoringPlatform.AGENT_OPS(with underscore) to match the enum definition insrc/agentunit/production/integrations.py.Based on relevant code snippets showing the pattern used by LangSmithAdapter and WandbAdapter.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/agentunit/adapters/agentops_adapter.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/agentunit/adapters/agentops_adapter.py (4)
src/agentunit/adapters/langsmith_adapter.py (1)
platform(104-106)src/agentunit/adapters/wandb_adapter.py (1)
platform(117-119)src/agentunit/production/integrations.py (1)
MonitoringPlatform(33-41)src/agentunit/reporting/results.py (1)
ScenarioResult(32-66)
🔇 Additional comments (4)
src/agentunit/adapters/agentops_adapter.py (4)
245-245: LGTM! Default to "broadcast" addresses validation concern.The fallback
to_agent or "broadcast"correctly prevents theValueError("to_agent is required")that would occur with an empty string, and aligns with the broadcast semantics used in other adapters.
348-348: LGTM! Explicit type annotation improves clarity.Adding the explicit type hint
dict[str, int]foragent_countsimproves code clarity and helps with type checking.
70-71: LGTM! Good type hint additions.Extracting
enable_tracingfrom kwargs and adding the explicit type annotationself.client: Anyimproves code clarity. WhileAnyis broad, it's reasonable for a dynamically-typed client library.
378-378: No changes needed - async signature is correct and required by MultiAgentAdapter abstract method.The
run_scenariomethod must beasyncbecause it implements the abstract method fromMultiAgentAdapter(line 372 insrc/agentunit/multiagent/__init__.py), which definesasync def run_scenario. This is not a premature change—it is the required implementation signature for the base class. No internal callers were found because this is an abstract method implemented by subclasses; external consumers would already be designed to handle async execution when using these adapters.Likely an incorrect or invalid review comment.
aviralgarg05
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls fix the issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/agentunit/adapters/agentops_adapter.py (4)
570-634: Remove unreachable code afterraise NotImplementedError.Lines 573-634 are unreachable after the
raisestatement. This dead code should be removed to improve maintainability. If retained for future reference, move it to a separate method or document it elsewhere.🔧 Proposed fix
def collect_metrics(self, scenario: Any, result: Any, **kwargs) -> ProductionMetrics: """ Collect production metrics from AgentOps. Args: scenario: The scenario being evaluated result: The result of the scenario execution **kwargs: Additional metadata Returns: ProductionMetrics: Current production metrics """ - raise NotImplementedError(_LANGSMITH_UNSUPPORTED) - - try: - # Query recent runs from AgentOps - runs = list(self.client.list_runs(project_name=self.project_id, limit=100)) - ... (remove all lines through 634)
636-703: Remove unreachable code and update docstring.
- The docstring at line 640 still references "LangSmith" - update to "AgentOps"
- Lines 652-703 are unreachable after
raise NotImplementedErrorand should be removed🔧 Proposed fix for docstring
def establish_baseline( self, historical_data: list[dict[str, Any]], metrics: list[str], **kwargs ) -> BaselineMetrics: """ - Establish baseline metrics from historical LangSmith data. + Establish baseline metrics from historical AgentOps data.
753-776: Remove unreachable code.Lines 756-776 are unreachable after
raise NotImplementedError. Additionally, log messages at lines 771 and 775 reference "LangSmith" which is inconsistent with this being the AgentOps adapter.
790-807: Remove unreachable code.Lines 793-807 are unreachable after
raise NotImplementedError. This block also imports fromlangsmith.evaluation(line 794), which would fail if LangSmith isn't installed and is inconsistent with the AgentOps adapter purpose.🔧 Proposed consolidated fix for all unreachable code
For
collect_metrics,establish_baseline,create_evaluation_dataset, andrun_evaluation, truncate each method after theraise NotImplementedErrorline:def collect_metrics(self, scenario: Any, result: Any, **kwargs) -> ProductionMetrics: """Collect production metrics from AgentOps.""" raise NotImplementedError(_LANGSMITH_UNSUPPORTED) - # Remove all code after raise def establish_baseline( self, historical_data: list[dict[str, Any]], metrics: list[str], **kwargs ) -> BaselineMetrics: """Establish baseline metrics from historical AgentOps data.""" raise NotImplementedError(_LANGSMITH_UNSUPPORTED) - # Remove all code after raise def create_evaluation_dataset( self, name: str, examples: list[dict[str, Any]], description: str | None = None ) -> str: """Create an evaluation dataset in AgentOps.""" raise NotImplementedError(_LANGSMITH_UNSUPPORTED) - # Remove all code after raise def run_evaluation(self, dataset_id: str, evaluator_function: Any, **kwargs) -> Any: """Run evaluation on an AgentOps dataset.""" raise NotImplementedError(_LANGSMITH_UNSUPPORTED) - # Remove all code after raise
🤖 Fix all issues with AI agents
In `@src/agentunit/adapters/agentops_adapter.py`:
- Around line 536-541: In the exception handler where you build trace_metadata
for AgentOps runs, the success flag is incorrectly set to True; update the
trace_metadata dictionary (the variable named trace_metadata that currently
includes "scenario_name": scenario.name, "metrics":
session_summary.get("metrics", {}), "success": True) to set "success": False so
failed scenarios correctly report failure; ensure this change is made in the
exception handling block that updates AgentOps run with error.
- Around line 75-88: Remove the dead commented LangSmith initialization block
that duplicates attribute handling; specifically delete the commented lines
referring to api_key, project_id, endpoint, enable_tracing, enable_feedback and
the assignments to self.api_key, self.project_id, self.default_tags, and
self.auto_start_session so only the active __init__ attribute assignments (the
existing code handling api_key/project_id/default_tags/auto_start_session around
the AgentOpsAdapter constructor) remain.
🧹 Nitpick comments (2)
src/agentunit/adapters/agentops_adapter.py (2)
386-389: Remove or clarify the misleading warning.The warning states "LangSmith-style APIs which are not fully supported by AgentOps" but the code now correctly uses AgentOps APIs (
start_trace(trace_name=..., tags=...),update_trace_metadata(...),end_trace(...)). This warning is outdated and may confuse maintainers.🔧 Proposed fix
- if self.enable_tracing: - logger.warning( - "run_scenario tracing uses LangSmith-style APIs which are not fully supported by AgentOps" - ) - logger.info(f"Running scenario with AgentOps: {scenario.name}")
553-554: Variable shadowing: inner exception shadows outer exception.The exception variable
eat line 553 shadows the outerefrom line 510. While the scope is limited here, renaming totrace_error similar improves clarity.🔧 Proposed fix
- except Exception as e: - logger.warning(f"Failed to finalize failed scenario trace: {e}") + except Exception as trace_err: + logger.warning(f"Failed to finalize failed scenario trace: {trace_err}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/agentunit/adapters/agentops_adapter.py`:
- Around line 522-538: The error path assumes session_summary exists and builds
trace_metadata but neither is guarded or used; to fix, ensure session_summary is
safely initialized or defaulted before use (e.g., if session_summary is
undefined set session_summary = {} or use session_summary.get(...) with a safe
default), use the constructed trace_metadata when calling
self.agentops.update_trace_metadata (pass trace_metadata or merge it with
{"error": str(e)}), and keep the existing calls to
self.agentops.update_trace_metadata and self.agentops.end_trace inside the
trace_log and self.enable_tracing guard so agentops.update_trace_metadata,
agentops.end_trace, session_summary, trace_metadata, trace_log, and
enable_tracing are all referenced and handled safely.
🧹 Nitpick comments (2)
src/agentunit/adapters/agentops_adapter.py (2)
544-621: Unreachable LangSmith blocks afterNotImplementedError.
These blocks will never run; consider removing them or gating behind a feature flag to avoid dead code and reduce maintenance surface.Also applies to: 636-690
725-794: Evaluation methods still contain unreachable LangSmith code.
Same pattern as above — consider removing or isolating the dead code to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agentunit/adapters/agentops_adapter.py (1)
362-544: Removeasynckeyword—method is fully synchronous and called withoutawait.The method is declared
async defbut the body contains no awaits and calls only synchronous methods. When called atcore/runner.py:51withoutawait, this returns a coroutine object instead of aScenarioResult, causing a type mismatch and runtime failure. Either removeasyncto match the synchronous implementation, or refactor the body to be truly asynchronous and update call sites to await the result.Note: Other adapters (LangSmith, Wandb) implement
run_scenarioas synchronousdef, violating the abstract method's async signature. This inconsistency should also be resolved.
🧹 Nitpick comments (1)
src/agentunit/adapters/agentops_adapter.py (1)
559-560: Unreachable code afterNotImplementedError.
The early raises make the remaining method bodies dead; consider removing or gating them with a TODO/feature flag to avoid confusion.Also applies to: 638-639, 742-743, 779-780

Description
Please include a summary of the changes and the related issue. Include relevant motivation and context.
I fixed the type hints for the following files:
src/agentunit/adapters/agentops_adapter.py
src/agentunit/core/scenario.py
src/agentunit/datasets/builtins.py
src/agentunit/metrics/builtin.py
src/agentunit/production/integrations.py
Note: /metrics/builtin.py has one import-untyped error
Fixes #13
Type of Change
Please delete options that are not relevant:
Changes Made
Please provide a detailed list of changes:
Testing
Please describe the tests you ran to verify your changes:
pytest)Test Configuration
Test Results
Code Quality
pre-commit run --all-filesand addressed any issuesDocumentation
Breaking Changes
If this PR introduces breaking changes, please describe:
Dependencies
New dependencies:
package-name: Reason for addingPerformance Impact
Describe any performance implications:
Screenshots/Recordings (if applicable)
Add screenshots or recordings to help explain your changes:
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
Refactor
Public API
✏️ Tip: You can customize this high-level summary in your review settings.