Conversation
…e files Co-authored-by: bchen <bchen@fireworks.ai>
|
Cursor Agent can help with this pull request. Just |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses and resolves several Pyright type errors within the eval_protocol package to improve type accuracy and handle optional values and external imports. The changes focus on removing problematic import dependencies, updating return type signatures, and improving type handling for optional values.
- Removed dependency on
openai.types.CompletionUsageand updated method signatures to use native Python types - Improved type safety by handling optional values and adding proper type coercion
- Added type ignore comments for external dependencies that may not be available during development
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| eval_protocol/playback_policy.py | Removed OpenAI dependency, updated return signature to use Optional types, removed duplicate method |
| eval_protocol/utils/static_policy.py | Updated method signatures to align with base class changes and handle optional parameters |
| eval_protocol/rewards/accuracy_length.py | Added robust string coercion for response content and type casting for accuracy reward |
| eval_protocol/server.py | Added type ignore comments for external imports and safe casting for legacy tuple returns |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| # Import the base policy and types for proper recording functionality | ||
| from openai.types import CompletionUsage | ||
| from typing import Optional as _Optional |
There was a problem hiding this comment.
This import is unused and creates an unnecessary alias. The regular Optional import on line 19 is sufficient for the usage in this file.
| from typing import Optional as _Optional |
| elif isinstance(response.content, list) and response.content: | ||
| # Join text parts if provided as structured content | ||
| try: | ||
| text = " ".join(part.text for part in response.content) # type: ignore[union-attr] |
There was a problem hiding this comment.
The type ignore comment suggests uncertainty about the structure. Consider adding a more specific type check or explicit attribute validation before accessing .text to make the code more robust.
| text = " ".join(part.text for part in response.content) # type: ignore[union-attr] | |
| text = " ".join( | |
| getattr(part, "text", "") for part in response.content if hasattr(part, "text") | |
| ) |
| ground_truth=ground_truth, # Pass the ground_truth list | ||
| # Ensure ground_truth is a list if provided; default to [] for compatibility | ||
| gt_for_accuracy = ground_truth if ground_truth is not None else [] | ||
| accuracy_eval_result = cast(Any, accuracy_reward)( |
There was a problem hiding this comment.
Casting to Any defeats the purpose of type checking. Consider using a more specific type or ensuring the decorator preserves the original function signature instead of using cast(Any, ...).
| accuracy_eval_result = cast(Any, accuracy_reward)( | |
| accuracy_eval_result = accuracy_reward( |
name: Pull Request
about: Propose changes to the codebase
title: "Fix: Address initial Pyright type errors in eval_protocol"
labels: ''
assignees: ''
Description
This PR addresses and resolves several initial Pyright type errors within the
eval_protocolpackage, reducing the overall type error count by more than five. The changes focus on improving type accuracy and handling optional values and external imports.Key changes include:
eval_protocol/playback_policy.py: Removedopenai.types.CompletionUsagedependency, updated the return signature ofgenerate_tool_callsto useOptional[Dict[str, int]]for usage andOptional[str]for correlation ID, and removed a duplicate method definition.eval_protocol/utils/static_policy.py: UpdatedStaticPolicyandRandomPolicyto align with the newPlaybackPolicyBasereturn signature forgenerate_tool_callsand madeinfoparametersOptional[Dict[str, Any]].eval_protocol/rewards/accuracy_length.py: Improved robustness by coercingresponse.contenttostr, handling optionalground_truthvalues, and adding acastforaccuracy_rewardto resolve decorator-related type inference issues.eval_protocol/server.py: Added a safecastfor legacy tuple return types from reward functions and introducedtype: ignorehints for missing external imports (uvicorn,fastapi,pydantic) to suppress errors related to uninstalled development dependencies.Type of change
How Has This Been Tested?
The changes were verified by running the
basedpyrighttype checker:eval_protocol/playback_policy.py,eval_protocol/utils/static_policy.py,eval_protocol/rewards/accuracy_length.py,eval_protocol/server.py).eval_protocolpackage to confirm a reduction in overall type errors.Test Configuration:
basedpyright(via pre-commit hook)Checklist:
black .,isort .,flake8 .)Screenshots (if applicable)
N/A
Additional context
This PR addresses the initial set of type errors as requested. Further work could include:
Union[str | List[ChatCompletionContentPartTextParam]]string-coercion issues in other reward files (e.g.,length.py,math.py,language_consistency.py).