Feature/error message #27
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EvalTag
participant Settings
participant RemoteService
User->>EvalTag: Instantiate with eval_name, config, mapping, model
EvalTag->>Settings: get_custom_eval_template(eval_name)
Settings->>RemoteService: POST /api/collector/v1/eval-template with eval_name
RemoteService-->>Settings: Return template config/mapping
Settings-->>EvalTag: Return template or empty dict
EvalTag->>EvalTag: Validate eval_name, config, mapping, model
EvalTag-->>User: EvalTag object ready
Possibly related PRs
Suggested reviewers
Poem
✨ 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. 🪧 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
python/fi_instrumentation/fi_types.py (1)
980-993: Fix f-string and simplify nested conditions.Apply this diff to fix the issues:
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." ) - if not is_custom_eval: - if not isinstance(self.eval_name, EvalName): + 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)}" ) return🧰 Tools
🪛 Ruff (0.11.9)
984-984: f-string without any placeholders
Remove extraneous
fprefix(F541)
987-988: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
python/fi_instrumentation/fi_types.py(7 hunks)python/fi_instrumentation/otel.py(3 hunks)python/fi_instrumentation/settings.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
python/fi_instrumentation/otel.py (1)
python/fi_instrumentation/fi_types.py (1)
EvalName(434-489)
python/fi_instrumentation/fi_types.py (1)
python/fi_instrumentation/settings.py (2)
get_env_collector_endpoint(15-16)get_custom_eval_template(106-136)
🪛 Ruff (0.11.9)
python/fi_instrumentation/settings.py
136-136: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
python/fi_instrumentation/fi_types.py
902-902: f-string without any placeholders
Remove extraneous f prefix
(F541)
921-923: Use a single if statement instead of nested if statements
(SIM102)
984-984: f-string without any placeholders
Remove extraneous f prefix
(F541)
987-988: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🔇 Additional comments (1)
python/fi_instrumentation/otel.py (1)
71-73: LGTM!The conversion of
eval_namefromEvalNameenum to its string value ensures consistent handling of evaluation names before processing.
Comment on lines
106
to
136
| 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}") |
There was a problem hiding this comment.
Improve exception handling and fix formatting issues.
The function has a few issues to address:
- Exception chaining should be used to preserve the original exception context
- Remove unnecessary blank line
- Fix inconsistent spacing in the Content-Type header
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",
+ "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.
Suggested change
| 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}") | |
| 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 except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In python/fi_instrumentation/settings.py between lines 106 and 136, improve
exception handling by using exception chaining with 'from e' when raising the
ValueError to preserve the original exception context. Remove the unnecessary
blank line after setting base_url. Fix the inconsistent spacing in the
Content-Type header by ensuring there is no extra space before the colon.
| type: EvalTagType | ||
| value: EvalSpanKind | ||
| eval_name: EvalName | ||
| eval_name: str | EvalName |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Union for Python <3.10 compatibility.
The pipe syntax for unions is only available in Python 3.10+. For broader compatibility, use Union from the typing module.
Add the import at the top of the file:
-from typing import Any, Dict, List, Type
+from typing import Any, Dict, List, Type, UnionThen update the type annotation:
- eval_name: str | EvalName
+ eval_name: Union[str, EvalName]🤖 Prompt for AI Agents
In python/fi_instrumentation/fi_types.py at line 879, replace the pipe syntax
used for the union type annotation with the Union type from the typing module
for compatibility with Python versions earlier than 3.10. Import Union from
typing at the top of the file and update the type annotation to use Union[str,
EvalName] instead of str | EvalName.
Comment on lines
900
to
936
| if not self.eval_name: | ||
| raise ValueError( | ||
| f"eval_name must be an EvalName enum, got {type(self.eval_name)}" | ||
| f"eval_name is required" | ||
| ) | ||
|
|
||
| if not self.custom_eval_name: | ||
| self.custom_eval_name = self.eval_name if isinstance(self.eval_name, str) else self.eval_name.value | ||
|
|
||
|
|
||
|
|
||
| eval_template = get_custom_eval_template(self.eval_name if isinstance(self.eval_name, str) else self.eval_name.value) | ||
| is_custom_eval = eval_template.get('isUserEvalTemplate') | ||
| custom_eval = eval_template.get('evalTemplate', {}) | ||
|
|
||
| self.validate_fagi_system_eval_name(is_custom_eval) | ||
|
|
||
| if is_custom_eval: | ||
| required_keys = custom_eval.get('config', {}).get('requiredKeys', []) | ||
| else: | ||
| required_keys = EvalMappingConfig.get_mapping_for_eval(self.eval_name).keys() | ||
|
|
||
| if not is_custom_eval: | ||
|
|
||
| if not isinstance(self.model, ModelChoices): | ||
| if (isinstance(self.model, str)): | ||
| valid_models = [model.value for model in ModelChoices] | ||
| if self.model not in valid_models: | ||
| raise ValueError( | ||
| f"model must be a valid model name, got {self.model}. Expected values are: {valid_models}" | ||
| ) | ||
| else: | ||
| self.model = ModelChoices(self.model) | ||
| else: | ||
| raise ValueError( | ||
| f"model must be a of type ModelChoices, got {type(self.model)}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify validation logic and fix f-string issues.
Several improvements can be made to this validation logic:
- Remove unnecessary f-string prefixes where no placeholders are used
- Simplify nested if statements for better readability
Apply these improvements:
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 if isinstance(self.eval_name, str) else self.eval_name.value
eval_template = get_custom_eval_template(self.eval_name if isinstance(self.eval_name, str) else self.eval_name.value)
is_custom_eval = eval_template.get('isUserEvalTemplate')
custom_eval = eval_template.get('evalTemplate', {})
self.validate_fagi_system_eval_name(is_custom_eval)
if is_custom_eval:
required_keys = custom_eval.get('config', {}).get('requiredKeys', [])
else:
required_keys = EvalMappingConfig.get_mapping_for_eval(self.eval_name).keys()
if not is_custom_eval:
- if not isinstance(self.model, ModelChoices):
- if (isinstance(self.model, str)):
+ if not isinstance(self.model, ModelChoices) and isinstance(self.model, str):
valid_models = [model.value for model in ModelChoices]
if self.model not in valid_models:
raise ValueError(
f"model must be a valid model name, got {self.model}. Expected values are: {valid_models}"
)
else:
self.model = ModelChoices(self.model)
- else:
+ elif not isinstance(self.model, ModelChoices):
raise ValueError(
- f"model must be a of type ModelChoices, got {type(self.model)}"
+ f"model must be of type ModelChoices, got {type(self.model)}"
)📝 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.
Suggested change
| if not self.eval_name: | |
| raise ValueError( | |
| f"eval_name must be an EvalName enum, got {type(self.eval_name)}" | |
| f"eval_name is required" | |
| ) | |
| if not self.custom_eval_name: | |
| self.custom_eval_name = self.eval_name if isinstance(self.eval_name, str) else self.eval_name.value | |
| eval_template = get_custom_eval_template(self.eval_name if isinstance(self.eval_name, str) else self.eval_name.value) | |
| is_custom_eval = eval_template.get('isUserEvalTemplate') | |
| custom_eval = eval_template.get('evalTemplate', {}) | |
| self.validate_fagi_system_eval_name(is_custom_eval) | |
| if is_custom_eval: | |
| required_keys = custom_eval.get('config', {}).get('requiredKeys', []) | |
| else: | |
| required_keys = EvalMappingConfig.get_mapping_for_eval(self.eval_name).keys() | |
| if not is_custom_eval: | |
| if not isinstance(self.model, ModelChoices): | |
| if (isinstance(self.model, str)): | |
| valid_models = [model.value for model in ModelChoices] | |
| if self.model not in valid_models: | |
| raise ValueError( | |
| f"model must be a valid model name, got {self.model}. Expected values are: {valid_models}" | |
| ) | |
| else: | |
| self.model = ModelChoices(self.model) | |
| else: | |
| raise ValueError( | |
| f"model must be a of type ModelChoices, got {type(self.model)}" | |
| ) | |
| if not self.eval_name: | |
| raise ValueError( | |
| "eval_name is required" | |
| ) | |
| if not self.custom_eval_name: | |
| self.custom_eval_name = self.eval_name if isinstance(self.eval_name, str) else self.eval_name.value | |
| eval_template = get_custom_eval_template(self.eval_name if isinstance(self.eval_name, str) else self.eval_name.value) | |
| is_custom_eval = eval_template.get('isUserEvalTemplate') | |
| custom_eval = eval_template.get('evalTemplate', {}) | |
| self.validate_fagi_system_eval_name(is_custom_eval) | |
| if is_custom_eval: | |
| required_keys = custom_eval.get('config', {}).get('requiredKeys', []) | |
| else: | |
| required_keys = EvalMappingConfig.get_mapping_for_eval(self.eval_name).keys() | |
| if not is_custom_eval: | |
| if not isinstance(self.model, ModelChoices) and isinstance(self.model, str): | |
| valid_models = [model.value for model in ModelChoices] | |
| if self.model not in valid_models: | |
| raise ValueError( | |
| f"model must be a valid model name, got {self.model}. Expected values are: {valid_models}" | |
| ) | |
| else: | |
| self.model = ModelChoices(self.model) | |
| elif not isinstance(self.model, ModelChoices): | |
| raise ValueError( | |
| f"model must be of type ModelChoices, got {type(self.model)}" | |
| ) |
🧰 Tools
🪛 Ruff (0.11.9)
902-902: f-string without any placeholders
Remove extraneous f prefix
(F541)
921-923: Use a single if statement instead of nested if statements
(SIM102)
🤖 Prompt for AI Agents
In python/fi_instrumentation/fi_types.py around lines 900 to 936, simplify the
validation logic by removing f-string prefixes from strings that do not contain
placeholders, and refactor nested if statements to reduce complexity and improve
readability. Specifically, replace f-strings without variables with regular
strings and combine or flatten nested conditionals where possible to make the
code clearer and easier to follow.
JayaSurya-27
approved these changes
May 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
Description
Describe the changes in this pull request:
Checklist
Related Issues
Closes #<issue_number>
Summary by CodeRabbit