Bugfix/model #26
Bugfix/model #26
Conversation
|
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 update refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EvalTag
participant Settings
participant RemoteService
User->>EvalTag: Instantiate EvalTag (with eval_name, config, mapping, etc.)
EvalTag->>Settings: get_custom_eval_template(eval_name)
Settings->>RemoteService: POST /custom_eval_template {eval_template_name}
RemoteService-->>Settings: Respond with template or error
Settings-->>EvalTag: Return template or raise error
EvalTag->>EvalTag: Validate config and mapping (custom vs standard)
EvalTag-->>User: EvalTag instance ready or error raised
Possibly related PRs
Suggested reviewers
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 (
|
![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: 3
🧹 Nitpick comments (1)
python/fi_instrumentation/fi_types.py (1)
894-1017: Consider refactoring validation logic for better maintainability.The validation logic in
__post_init__is comprehensive but quite complex. Consider extracting the custom eval template fetching and validation into separate methods for better readability and testability.Example structure:
def __post_init__(self): # Initialize defaults self._initialize_defaults() # Fetch and validate eval template eval_template = self._fetch_eval_template() is_custom_eval = self._is_custom_eval(eval_template) # Validate based on eval type if is_custom_eval: self._validate_custom_eval(eval_template) else: self._validate_standard_eval() def _fetch_eval_template(self) -> Dict[str, Any]: """Fetch eval template with proper error handling.""" try: return get_custom_eval_template(self.eval_name) except Exception: return {} def _is_custom_eval(self, eval_template: Dict[str, Any]) -> bool: """Determine if this is a custom eval.""" return eval_template.get('isUserEvalTemplate', False)🧰 Tools
🪛 Ruff (0.11.9)
910-910: f-string without any placeholders
Remove extraneous
fprefix(F541)
923-924: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
926-926: f-string without any placeholders
Remove extraneous
fprefix(F541)
977-977: f-string without any placeholders
Remove extraneous
fprefix(F541)
🛑 Comments failed to post (3)
python/fi_instrumentation/settings.py (1)
106-136:
⚠️ Potential issueImprove exception handling and remove unnecessary empty line.
The function implementation looks good, but there are a few improvements to make:
- Use
raise ... from efor better exception chaining- Remove the unnecessary empty line at line 118
Apply this diff to fix the issues:
def get_custom_eval_template( eval_name: str, base_url: Optional[str] = None ) -> Dict[str, Any]: """ Check if a custom eval template exists for a given eval name. """ if not eval_name: raise ValueError("Eval name is required") if base_url is None: base_url = get_env_collector_endpoint() - url = f"{base_url}/tracer/custom-eval-config/get_custom_eval_by_name/" try: headers = { "Content-Type" : "application/json", **(get_env_fi_auth_header() or {}), } response = requests.post( url, headers=headers, json={"eval_template_name": eval_name}, ) response.raise_for_status() return response.json().get("result", {}) except Exception as e: - raise ValueError(f"Failed to check custom eval template: {e}") + raise ValueError(f"Failed to check 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.def get_custom_eval_template( eval_name: str, base_url: Optional[str] = None ) -> Dict[str, Any]: """ Check if a custom eval template exists for a given eval name. """ if not eval_name: raise ValueError("Eval name is required") if base_url is None: base_url = get_env_collector_endpoint() url = f"{base_url}/tracer/custom-eval-config/get_custom_eval_by_name/" try: headers = { "Content-Type": "application/json", **(get_env_fi_auth_header() or {}), } response = requests.post( url, headers=headers, json={"eval_template_name": eval_name}, ) response.raise_for_status() return response.json().get("result", {}) except Exception as e: raise ValueError(f"Failed to check 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 from lines 106 to 136, improve exception handling by changing the raise statement in the except block to use "raise ValueError(...) from e" for proper exception chaining. Also, remove the unnecessary empty line at line 118 to clean up the code.python/fi_instrumentation/fi_types.py (2)
973-988:
⚠️ Potential issueFix f-string formatting in validation method.
Remove unnecessary f-string prefix where no placeholder is used.
Apply this diff:
def validate_fagi_system_eval_name(self, is_custom_eval: bool) -> None: if not self.eval_name: raise ValueError( - f"eval_name must be an Present." + "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.def validate_fagi_system_eval_name(self, is_custom_eval: bool) -> None: if not self.eval_name: raise ValueError( "eval_name must be present." ) if not is_custom_eval: eval_names = [e.value for e in EvalName] if self.eval_name not in eval_names: raise ValueError( f"eval_name {self.eval_name} is not a valid eval name" ) return🧰 Tools
🪛 Ruff (0.11.9)
977-977: f-string without any placeholders
Remove extraneous
fprefix(F541)
🤖 Prompt for AI Agents
In python/fi_instrumentation/fi_types.py around lines 973 to 988, the f-string in the ValueError message "eval_name must be an Present." is unnecessary since it contains no placeholders. Remove the f-string prefix (the leading 'f') from this string to correct the formatting.
908-928:
⚠️ Potential issueFix f-string formatting and improve error handling.
Several issues need to be addressed:
- Remove unnecessary f-string prefixes where no placeholders are used
- Combine nested if statements for better readability
- Add error handling for the API call
Apply this diff to fix the issues:
- if not self.eval_name: + if not self.eval_name: raise ValueError( - f"eval_name is required" + "eval_name is required" ) if not self.custom_eval_name: self.custom_eval_name = self.eval_name - eval_template = get_custom_eval_template(self.eval_name) - is_custom_eval = eval_template.get('isUserEvalTemplate') - custom_eval = eval_template.get('evalTemplate', {}) - if custom_eval: - required_keys = custom_eval.get('config', {}).get('requiredKeys', []) + try: + eval_template = get_custom_eval_template(self.eval_name) + except Exception as e: + # If the API call fails, assume it's not a custom eval + eval_template = {} + + is_custom_eval = eval_template.get('isUserEvalTemplate', False) + custom_eval = eval_template.get('evalTemplate', {}) + required_keys = [] + if custom_eval and 'config' in custom_eval: + required_keys = custom_eval.get('config', {}).get('requiredKeys', []) - if not is_custom_eval: - if not isinstance(self.model, ModelChoices): - raise ValueError( - f"model must be a present for all non-custom evals" - ) + if not is_custom_eval and not isinstance(self.model, ModelChoices): + raise ValueError( + "model must be present for all non-custom evals" + )📝 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" ) if not self.custom_eval_name: self.custom_eval_name = self.eval_name try: eval_template = get_custom_eval_template(self.eval_name) except Exception as e: # If the API call fails, assume it's not a custom eval eval_template = {} is_custom_eval = eval_template.get('isUserEvalTemplate', False) custom_eval = eval_template.get('evalTemplate', {}) required_keys = [] if custom_eval and 'config' in custom_eval: required_keys = custom_eval.get('config', {}).get('requiredKeys', []) if not is_custom_eval and not isinstance(self.model, ModelChoices): raise ValueError( "model must be present for all non-custom evals" )🧰 Tools
🪛 Ruff (0.11.9)
910-910: f-string without any placeholders
Remove extraneous
fprefix(F541)
923-924: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
926-926: f-string without any placeholders
Remove extraneous
fprefix(F541)
🤖 Prompt for AI Agents
In python/fi_instrumentation/fi_types.py around lines 908 to 928, remove the f-string prefixes from error messages that do not contain placeholders to simplify the code. Combine the nested if statements checking for custom_eval and is_custom_eval into a single conditional block to improve readability. Additionally, wrap the call to get_custom_eval_template(self.eval_name) in a try-except block to handle potential exceptions gracefully, raising a clear error if the API call fails.
Pull Request
Description
Describe the changes in this pull request:
Checklist
Related Issues
Closes #<issue_number>
Summary by CodeRabbit
New Features
Bug Fixes
Chores