Bugfix/custom evals prototype #25
Conversation
…on handling. Updated eval_name type and adjusted related logic for consistency in mapping validation.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe updates introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EvalTag
participant Settings
participant RemoteService
User->>EvalTag: Instantiate with eval_name, model, config, mapping
EvalTag->>Settings: get_custom_eval_template(eval_name)
Settings->>RemoteService: POST /custom_eval_template (with eval_name)
RemoteService-->>Settings: Return template or error
Settings-->>EvalTag: Return template/config
EvalTag->>EvalTag: Validate eval_name, config, mapping
EvalTag-->>User: EvalTag instance ready or error raised
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…n EvalConfig and EvalMappingConfig to streamline evaluation types.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (3)
python/fi_instrumentation/settings.py (2)
118-118: Remove extra blank line.Remove the extra blank line for consistency with the codebase style:
- url = f"{base_url}/tracer/custom-eval-config/get_custom_eval_by_name/"
119-119: Consider making the API endpoint configurable.The hardcoded path
/tracer/custom-eval-config/get_custom_eval_by_name/might need to be updated if the API changes. Consider making this configurable or defining it as a constant.python/fi_instrumentation/fi_types.py (1)
982-986: Simplify nested if statements.The nested if statements can be combined for better readability.
Apply this diff:
- if not is_custom_eval: - if not isinstance(self.eval_name, EvalName): - raise ValueError( - f"eval_name must be an EvalName enum, got {type(self.eval_name)}" - ) + if not is_custom_eval and not isinstance(self.eval_name, EvalName): + raise ValueError( + f"eval_name must be an EvalName enum, got {type(self.eval_name)}" + )🧰 Tools
🪛 Ruff (0.11.9)
982-983: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
🛑 Comments failed to post (4)
python/fi_instrumentation/settings.py (1)
135-136:
⚠️ Potential issueImprove exception handling with explicit chaining.
The static analysis correctly identifies that exceptions should be chained to preserve the original traceback and distinguish them from errors in exception handling.
Additionally, consider catching more specific exceptions like
requests.RequestExceptioninstead of the genericException.Apply this diff to improve exception handling:
- except Exception as e: - raise ValueError(f"Failed to check custom eval template: {e}") + except requests.RequestException as e: + raise ValueError(f"Failed to check custom eval template: {e}") from e + except Exception as e: + raise ValueError(f"Unexpected error checking custom eval template: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.except requests.RequestException as e: raise ValueError(f"Failed to check custom eval template: {e}") from e except Exception as e: raise ValueError(f"Unexpected error checking custom eval template: {e}") from e🧰 Tools
🪛 Ruff (0.11.9)
136-136: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🤖 Prompt for AI Agents
In python/fi_instrumentation/settings.py around lines 135 to 136, replace the generic Exception catch with a more specific exception such as requests.RequestException, and use explicit exception chaining by adding "from e" when raising the new ValueError to preserve the original traceback and improve error clarity.python/fi_instrumentation/fi_types.py (3)
921-924: 🛠️ Refactor suggestion
Consider the performance impact of network calls in
__post_init__.Making a network call to
get_custom_eval_templateduring object initialization could cause performance issues and unexpected failures. Consider:
- Caching the results to avoid repeated API calls
- Making this validation optional or lazy
- Handling network failures gracefully
🤖 Prompt for AI Agents
In python/fi_instrumentation/fi_types.py around lines 921 to 924, the __post_init__ method makes a network call to get_custom_eval_template which can degrade performance and cause failures during object initialization. Refactor this by implementing caching to store and reuse results of get_custom_eval_template, or defer the call to a lazy evaluation method that runs only when needed. Additionally, add error handling to gracefully manage network failures without crashing the initialization process.
977-980:
⚠️ Potential issueFix f-string without placeholders.
The f-string on line 979 doesn't contain any placeholders.
Apply this diff:
- if not self.eval_name: - raise ValueError( - f"eval_name must be an Present." - ) + if not self.eval_name: + raise ValueError("eval_name must be Present.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if not self.eval_name: raise ValueError("eval_name must be Present.")🧰 Tools
🪛 Ruff (0.11.9)
979-979: f-string without any placeholders
Remove extraneous
fprefix(F541)
🤖 Prompt for AI Agents
In python/fi_instrumentation/fi_types.py around lines 977 to 980, the f-string used in the ValueError message does not contain any placeholders and is unnecessary. Replace the f-string with a regular string literal by removing the leading 'f' before the quotes in the raise statement.
908-911:
⚠️ Potential issueFix f-string without placeholders.
The f-string on line 910 doesn't contain any placeholders.
Apply this diff:
- if not self.eval_name: - raise ValueError( - f"eval_name is required" - ) + if not self.eval_name: + raise ValueError("eval_name is required")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if not self.eval_name: raise ValueError("eval_name is required")🧰 Tools
🪛 Ruff (0.11.9)
910-910: f-string without any placeholders
Remove extraneous
fprefix(F541)
🤖 Prompt for AI Agents
In python/fi_instrumentation/fi_types.py around lines 908 to 911, the f-string used in the ValueError message does not contain any placeholders, making the f-string unnecessary. Replace the f-string with a regular string literal by removing the leading 'f' before the quotes in the raise statement.
Pull Request
Description
Describe the changes in this pull request:
Checklist
Related Issues
Closes #<issue_number>
Summary by CodeRabbit