packages/traceloop-sdk/traceloop/sdk/client/http.py (1)

97-130: Tighten _format_error_message exception handling and address linter hints

The helper is a solid improvement, but a couple of small tweaks will both satisfy the linters and make behavior clearer:

• Line 101 (RUF010): you can avoid the f-string conversion warning and the extra str() call by using an explicit conversion flag:

•    error_parts = [f"Error making request to {path}: {str(exception)}"]
  

•    error_parts = [f"Error making request to {path}: {exception!s}"]
  

• Lines 123–128 (S110, BLE001): catching bare Exception and doing pass is a code smell. Narrowing the exception type and handling it explicitly (even in a minimal way) makes the intent clearer and should quiet the warnings. For example:

             except (ValueError, AttributeError):
                 # Not JSON, try to get text
                 try:
                     error_text = response.text
                     if error_text and len(error_text) < 500:  # Only include short error messages
                         error_parts.append(f"Server response: {error_text}")

•            except Exception:
  

•                pass
  

•            except (AttributeError, UnicodeDecodeError) as exc:
  

•                # Don't let response text decoding failures break error formatting
  

•                error_parts.append(f"Failed to read server error text: {exc!s}")
  

  (Adjust the caught exception types if you see other concrete failures in practice.)
  
  

Optionally, you might also append the HTTP status code when exception.response is present to further improve debuggability, but the current behavior is already a clear upgrade.

packages/sample-app/sample_app/experiment/made_by_traceloop/style_exp.py (1)

90-98: Prefix unused unpacked variables with underscore.

The results and errors variables are unpacked but never used. Following Python conventions, prefix them with underscores to indicate intentional non-use.

Apply this diff:

-    results, errors = await client.experiment.run(
+    _results, _errors = await client.experiment.run(

packages/sample-app/sample_app/experiment/made_by_traceloop/compliance_exp.py (1)

88-96: Prefix unused unpacked variables with underscore.

The results and errors variables are unpacked but never used. Following Python conventions, prefix them with underscores to indicate intentional non-use.

Apply this diff:

-    results, errors = await client.experiment.run(
+    _results, _errors = await client.experiment.run(

packages/sample-app/sample_app/experiment/made_by_traceloop/quality_exp.py (1)

104-112: Prefix unused unpacked variables with underscore.

The results and errors variables are unpacked but never used. Following Python conventions, prefix them with underscores to indicate intentional non-use.

Apply this diff:

-    results, errors = await client.experiment.run(
+    _results, _errors = await client.experiment.run(

packages/sample-app/sample_app/experiment/made_by_traceloop/correctness_exp.py (2)

22-22: Use explicit Optional type hint.

PEP 484 prohibits implicit Optional. The parameter should explicitly declare the optional type.

+from typing import Optional
+
-async def generate_response(prompt: str, context: str = None) -> str:
+async def generate_response(prompt: str, context: Optional[str] = None) -> str:

---

90-98: Prefix unused variables with underscore.

The results and errors variables are unpacked but never used. As flagged by static analysis (Ruff RUF059), prefix them with underscores.

-    results, errors = await client.experiment.run(
+    _results, _errors = await client.experiment.run(

packages/sample-app/sample_app/experiment/made_by_traceloop/formatting_exp.py (2)

127-135: Prefix unused variables with underscore.

As flagged by static analysis (Ruff RUF059), prefix unused variables with underscores.

-    results, errors = await client.experiment.run(
+    _results, _errors = await client.experiment.run(

---

142-191: run_validation_examples() is defined but never called.

This function provides useful live examples but is never invoked from the main entry point. Consider either calling it (e.g., after run_formatting_experiment()) or removing it if it's not needed.

packages/sample-app/sample_app/experiment/made_by_traceloop/security_exp.py (1)

84-92: Prefix unused variables with underscore.

As flagged by static analysis (Ruff RUF059), prefix unused variables with underscores.

-    results, errors = await client.experiment.run(
+    _results, _errors = await client.experiment.run(

packages/traceloop-sdk/traceloop/sdk/evaluator/evaluator.py (1)

211-223: Consider logging the exception in the fallback path.

While the fallback to response.text is appropriate, Ruff (S110) suggests logging exceptions rather than silently passing. This could aid debugging when error extraction fails unexpectedly.

+import logging
+
+logger = logging.getLogger(__name__)
+
 def _extract_error_from_response(response: httpx.Response) -> str:
     # ...
     try:
         error_json = response.json()
         # ...
-    except Exception:
-        pass  # Use response.text as fallback
+    except Exception as e:
+        logger.debug("Failed to parse error response as JSON: %s", e)

     return error_detail

packages/traceloop-sdk/traceloop/sdk/experiment/experiment.py (3)

43-57: Make evaluators explicitly optional to match the default and avoid implicit Optional

evaluators has a default of None but is annotated as a non‑optional List[EvaluatorSpec], which both misleads type checkers and triggers Ruff’s RUF013 (“implicit Optional”). Align it with the other methods and the default:

-        evaluators: List[EvaluatorSpec] = None,
+        evaluators: Optional[List[EvaluatorSpec]] = None,

The updated docstring for evaluators already reflects the richer accepted types, so no further change needed there.

---

181-225: Simplify exception formatting in f-strings and consider using logging instead of print

The new error handling is welcome, but you’re wrapping e with str(e) inside f-strings:

error_msg = f"Error: {str(e)}"
print(f"\033[91m❌ Evaluator '{evaluator_slug}' failed: {str(e)}\033[0m")
error_msg = f"Error processing row: {str(e)}"
print(f"\033[91m❌ Task execution failed: {str(e)}\033[0m")

Given f-strings already call str() by default, and Ruff flags this as RUF010, you can simplify and satisfy the linter:

-                            error_msg = f"Error: {str(e)}"
+                            error_msg = f"Error: {e}"
...
-                            print(f"\033[91m❌ Evaluator '{evaluator_slug}' failed: {str(e)}\033[0m")
+                            print(f"\033[91m❌ Evaluator '{evaluator_slug}' failed: {e}\033[0m")
...
-                error_msg = f"Error processing row: {str(e)}"
+                error_msg = f"Error processing row: {e}"
...
-                print(f"\033[91m❌ Task execution failed: {str(e)}\033[0m")
+                print(f"\033[91m❌ Task execution failed: {e}\033[0m")

Longer term, you might want to route these through the standard logging framework instead of print for better control in libraries.

---

262-365: Align _run_in_github evaluator handling and docstring with supported types

_run_in_github now accepts evaluators: Optional[List[EvaluatorSpec]] and extracts slugs via:

if evaluators:
    evaluator_slugs = []
    for evaluator in evaluators:
        if isinstance(evaluator, str):
            evaluator_slugs.append(evaluator)
        elif isinstance(evaluator, EvaluatorDetails):
            evaluator_slugs.append(evaluator.slug)

while the docstring still says:

evaluators: List of evaluator slugs or (slug, version) tuples to run

Either reintroduce tuple support in the implementation or (more likely) update the docstring to mention str / EvaluatorDetails (or EvaluatorSpec) rather than (slug, version) tuples so users aren’t misled about accepted shapes.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: nina-kollman
Repo: traceloop/openllmetry PR: 3219
File: packages/traceloop-sdk/traceloop/sdk/datasets/dataset.py:357-360
Timestamp: 2025-08-04T15:35:30.188Z
Learning: In the traceloop SDK Dataset class, the established error handling pattern is that HTTP client methods return None on failure (after catching and logging RequestException), and the Dataset API methods check for None return values and raise Exception with descriptive messages. The update_row_api method is inconsistent with this pattern.

packages/traceloop-sdk/traceloop/sdk/experiment/experiment.py (2)

38-58: Tighten evaluators type annotation in run to match default None

run() declares evaluators: List[EvaluatorSpec] = None, which is an implicit Optional and what Ruff is flagging (RUF013). The rest of the file uses Optional[List[EvaluatorSpec]] when None is allowed.

To keep type checkers and linters happy and make the intent explicit, consider:

-        evaluators: List[EvaluatorSpec] = None,
+        evaluators: Optional[List[EvaluatorSpec]] = None,

---

454-502: Make _execute_tasks exception flow more idiomatic and preserve original tracebacks

In _execute_tasks.run_single_row:

• You catch ValueError from validate_task_output, print, then raise a new ValueError(str(validation_error)), losing the original traceback.
• In the outer except, you check isinstance(e, ValueError) and do raise e, which again resets the traceback (TRY201 / B904).
• Ruff also flags the str(...) inside the f-strings (RUF010).

You can both simplify the flow and keep original traces by re-raising the current exception and separating ValueError from other failures:

        async def run_single_row(row: Optional[Dict[str, Any]]) -> TaskResult:
             try:
                 task_output = await task(row)
 
-                # Validate task output schema on first execution
-                if evaluators_to_validate:
-                    try:
-                        validate_task_output(task_output, evaluators_to_validate)
-                    except ValueError as validation_error:
-                        print(f"\033[91m❌ Task validation failed: {str(validation_error)}\033[0m")
-                        raise ValueError(str(validation_error))
-
-                return TaskResult(
-                    input=row,
-                    output=task_output,
-                )
-            except Exception as e:
-                if isinstance(e, ValueError):
-                    raise e
-                print(f"\033[91m❌ Task execution error: {str(e)}\033[0m")
-                return TaskResult(
-                    input=row,
-                    error=str(e),
-                )
+                # Validate task output schema each execution (fail fast on mismatch)
+                if evaluators_to_validate:
+                    try:
+                        validate_task_output(task_output, evaluators_to_validate)
+                    except ValueError as validation_error:
+                        print(f"\033[91m❌ Task validation failed: {validation_error!s}\033[0m")
+                        # Re-raise original ValueError with its traceback
+                        raise
+
+                return TaskResult(
+                    input=row,
+                    output=task_output,
+                )
+            except ValueError:
+                # Propagate validation/type errors unchanged to abort the run
+                raise
+            except Exception as e:
+                print(f"\033[91m❌ Task execution error: {e!s}\033[0m")
+                return TaskResult(
+                    input=row,
+                    error=str(e),
+                )

Semantics stay the same (all ValueErrors still bubble out and cause run_in_github to fail), but you keep meaningful stack traces and address the static-analysis warnings.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

packages/traceloop-sdk/traceloop/sdk/evaluator/evaluator.py (1)

2-2: LGTM! Unused imports removed.

The unused imports flagged by Flake8 have been properly removed, keeping only the types actually used in this file.

packages/traceloop-sdk/traceloop/sdk/evaluator/evaluators_made_by_traceloop.py (4)

5-19: LGTM! Clear factory class design.

The class provides a clean factory pattern for creating preconfigured evaluators with proper documentation and examples.

---

21-78: LGTM! Detection evaluators properly configured.

The detector evaluators (PII, toxicity, prompt injection) are correctly implemented with matching docstrings and required_input_fields.

---

241-274: LGTM! RAG evaluators properly configured.

The answer_relevancy and faithfulness evaluators have correct docstrings matching their required_input_fields.

---

224-239: Remove stray "+" character on Line 232.

There's a "+" character at the beginning of Line 232 that appears to be a formatting artifact or merge leftover.

Apply this diff:

         Required task output fields:
            - numerator_text: The numerator text for ratio calculation
-          - denominator_text: The denominator text for ratio calculation
+           - denominator_text: The denominator text for ratio calculation

packages/traceloop-sdk/traceloop/sdk/evaluator/evaluator.py (1)

210-221: Consider narrowing the exception handler.

The broad except Exception is flagged by static analysis. While using a fallback here is appropriate, consider either catching json.JSONDecodeError specifically or adding a comment explaining why a broad exception handler is intentional.

Apply this diff to narrow the exception:

     error_detail = response.text
     try:
         error_json = response.json()
         if isinstance(error_json, dict):
             if 'error' in error_json:
                 error_detail = error_json['error']
             elif 'message' in error_json:
                 error_detail = error_json['message']
             elif 'msg' in error_json:
                 error_detail = error_json['msg']
-    except Exception:
-        pass  # Use response.text as fallback
+    except (ValueError, TypeError):
+        pass  # JSON parsing failed, use response.text as fallback

Alternatively, keep the broad exception but add a clarifying comment:

-    except Exception:
-        pass  # Use response.text as fallback
+    except Exception:
+        # Intentionally catch all exceptions to ensure error extraction never fails
+        # Falls back to response.text which is already set above
+        pass

Based on static analysis hints from Ruff.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

packages/traceloop-sdk/traceloop/sdk/evaluator/evaluators_made_by_traceloop.py (1)

235-246: Remove stray + and fix bullet indentation in word_count_ratio docstring.

The docstring line for denominator_text still has a leading +, and indentation for the bullets is inconsistent. This is a leftover from a diff and hurts readability, even though it doesn't affect runtime behavior.

Suggested fix:

         Required task output fields:
-           - numerator_text: The numerator text for ratio calculation
-+          - denominator_text: The denominator text for ratio calculation
+            - numerator_text: The numerator text for ratio calculation
+            - denominator_text: The denominator text for ratio calculation

packages/traceloop-sdk/traceloop/sdk/experiment/experiment.py (1)

484-490: Consider graceful handling of validation errors.

The current implementation re-raises ValueError on validation failure, which will abort all remaining tasks in GitHub execution mode. This could be unexpected if users want to process other rows despite validation failures in one row.

Additionally, the exception re-raise should use from for better exception chaining.

Apply this diff to improve exception chaining:

                     except ValueError as validation_error:
                         print(f"\033[91m❌ Task validation failed: {str(validation_error)}\033[0m")
-                        raise ValueError(str(validation_error))
+                        raise ValueError(str(validation_error)) from validation_error

Consider whether to:

1. Return TaskResult(input=row, error=str(validation_error)) instead of raising (as suggested in the past review)
2. Add a parameter to control whether validation failures should abort execution

packages/traceloop-sdk/traceloop/sdk/evaluator/evaluators_made_by_traceloop.py (1)

114-137: Consider enforcing schema_string when enable_schema_validation=True.

The docstring says schema_string is required when enable_schema_validation is True, but the implementation does not enforce this. To avoid silent misconfiguration, you could validate inputs:

     def json_validator(
         enable_schema_validation: bool = False,
         schema_string: Optional[str] = None,
     ) -> EvaluatorDetails:
@@
-        config: Dict[str, Any] = {
-            "enable_schema_validation": enable_schema_validation,
-        }
-        if schema_string:
-            config["schema_string"] = schema_string
+        config: Dict[str, Any] = {"enable_schema_validation": enable_schema_validation}
+        if enable_schema_validation and schema_string is None:
+            raise ValueError("schema_string is required when enable_schema_validation is True")
+        if schema_string is not None:
+            config["schema_string"] = schema_string

This keeps the runtime behavior aligned with the documented contract.

packages/traceloop-sdk/traceloop/sdk/experiment/experiment.py (1)

497-498: Use bare raise to preserve the original exception traceback.

When re-raising an exception without modification, use bare raise instead of raise e to maintain the complete traceback chain.

Apply this diff:

                 if isinstance(e, ValueError):
-                    raise e
+                    raise

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

packages/traceloop-sdk/traceloop/sdk/evaluator/evaluators_made_by_traceloop.py (1)

243-243: Remove the extra + character from the docstring.

Line 243 contains a stray + character at the beginning of the line, likely a leftover diff marker. This will appear in the rendered documentation.

Apply this diff to remove the extra character:

-+          - denominator_text: The denominator text for ratio calculation
+           - denominator_text: The denominator text for ratio calculation

packages/traceloop-sdk/traceloop/sdk/experiment/experiment.py (1)

455-503: _execute_tasks validation and ValueError handling may abort all tasks; also a few linter-aligned cleanups

The new _execute_tasks helper is a nice abstraction and the validation hook via validate_task_output is a good safeguard. A few points to tighten up:

1. ValueError handling aborts the entire GitHub run (previous concern still applies)

Current logic:

try:
    task_output = await task(row)
    if evaluators_to_validate:
        try:
            validate_task_output(task_output, evaluators_to_validate)
        except ValueError as validation_error:
            print(f"\033[91m❌ Task validation failed: {str(validation_error)}\033[0m")
            raise ValueError(str(validation_error))
    return TaskResult(...)
except Exception as e:
    if isinstance(e, ValueError):
        raise e
    print(f"\033[91m❌ Task execution error: {str(e)}\033[0m")
    return TaskResult(input=row, error=str(e))

• Any ValueError (from the task or from validation) is propagated out of _execute_tasks, aborting the loop at line 514 on the first failing row.
• That means later rows are never awaited, and GitHub runs become “fail fast” on ValueError.

This is very similar to the existing review comment at Line 490 questioning whether this should instead return a TaskResult with an error field. If you want behavior consistent with local execution (and to avoid partial processing), you could treat validation failures like other errors:

         if evaluators_to_validate:
             try:
                 validate_task_output(task_output, evaluators_to_validate)
             except ValueError as validation_error:
-                print(f"\033[91m❌ Task validation failed: {str(validation_error)}\033[0m")
-                raise ValueError(str(validation_error))
+                msg = str(validation_error)
+                print(f"\033[91m❌ Task validation failed: {msg}\033[0m")
+                return TaskResult(input=row, error=msg)

-    except Exception as e:
-        if isinstance(e, ValueError):
-            raise e
-        print(f"\033[91m❌ Task execution error: {str(e)}\033[0m")
-        return TaskResult(
-            input=row,
-            error=str(e),
-        )
+    except Exception as e:
+        print(f"\033[91m❌ Task execution error: {e!s}\033[0m")
+        return TaskResult(input=row, error=str(e))

This preserves validation and logging but avoids aborting the whole run on first ValueError.

2. Comment vs. implementation mismatch

The comment says:

# Validate task output schema on first execution

but validation runs on every row. Either adjust the comment to reflect that it validates each row, or short-circuit validation after the first success/failure if only schema shape is important.

3. Linter-style improvements (RUF010, B904, TRY201)

If you decide to keep the “fail fast on ValueError” behavior, you can still clean up the exception handling and prints:

• Prefer explicit conversion flags in f-strings:

- print(f"\033[91m❌ Task validation failed: {str(validation_error)}\033[0m")
+ print(f"\033[91m❌ Task validation failed: {validation_error!s}\033[0m")

- print(f"\033[91m❌ Task execution error: {str(e)}\033[0m")
+ print(f"\033[91m❌ Task execution error: {e!s}\033[0m")

• If you keep re-wrapping the ValueError, chain it:

- raise ValueError(str(validation_error))
+ raise ValueError(str(validation_error)) from validation_error

• If you keep the current branchy except logic, prefer a dedicated except ValueError for re-raising instead of raise e:

except ValueError:
    raise
except Exception as e:
    print(f"\033[91m❌ Task execution error: {e!s}\033[0m")
    return TaskResult(input=row, error=str(e))

These adjustments improve tracebacks and align with the static analysis hints without changing semantics.

packages/traceloop-sdk/traceloop/sdk/experiment/experiment.py (2)

91-226: Evaluator normalization and local validation flow look sound; minor robustness & logging tweaks

The new _run_locally flow correctly:

• Normalizes evaluators: List[EvaluatorSpec] into (slug, version, config) tuples for evaluator execution.
• Extracts EvaluatorDetails instances into evaluators_to_validate and uses validate_task_output before creating tasks.
• Passes evaluator_config through to both run_experiment_evaluator and trigger_experiment_evaluator.

A couple of small improvements to consider:

1. Defensive handling of unsupported EvaluatorSpec variants

Right now only str and EvaluatorDetails are handled:

for evaluator in evaluators:
    if isinstance(evaluator, str):
        ...
    elif isinstance(evaluator, EvaluatorDetails):
        ...

If EvaluatorSpec is extended in the future, such evaluators will silently be ignored. You may want to either:

• Raise a TypeError for unknown types, or
• At least log a warning so misconfigured specs are visible.

2. Use f-string conversion instead of str() for errors (RUF010)

For the new logging, you can simplify and satisfy the linter by using conversion flags instead of str() inside f-strings, e.g.:

- error_msg = f"Error: {str(e)}"
+ error_msg = f"Error: {e!s}"

- print(f"\033[91m❌ Evaluator '{evaluator_slug}' failed: {str(e)}\033[0m")
+ print(f"\033[91m❌ Evaluator '{evaluator_slug}' failed: {e!s}\033[0m")

- print(f"\033[91m❌ Task execution failed: {str(e)}\033[0m")
+ print(f"\033[91m❌ Task execution failed: {e!s}\033[0m")

Functionally everything here looks correct; these are just polish and future-proofing suggestions.

---

258-378: GitHub flow wiring to _execute_tasks and evaluator slugs is consistent, but error behavior now hinges on ValueError semantics

The GitHub path now:

• Executes tasks via _execute_tasks(rows, task, evaluators), which performs the same validation used locally.
• Derives evaluator_slugs from evaluators, supporting both str and EvaluatorDetails, then sends only slugs to the backend. This matches how _init_experiment is fed slugs locally.

One thing to be aware of: _execute_tasks re-raises any ValueError (see comment below), so in GitHub mode a ValueError from either:

• The task itself, or
• validate_task_output

will abort _run_in_github and fail the workflow, rather than returning a partial task_results list with per-row errors. That might be exactly what you want for CI, but it is a behavior change compared to local execution, where errors are wrapped in TaskResponse unless stop_on_error is set.

If the intent is to keep GitHub runs “all-or-nothing” on validation failures, this is fine; otherwise, consider aligning _execute_tasks’s ValueError handling with _run_locally (returning error-bearing TaskResult objects instead of aborting).

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

packages/traceloop-sdk/tests/evaluator/test_evaluator.py (2)

103-120: Simplify the assertion logic for clarity.

The assertion on Line 120 is logically correct but unnecessarily complex. Since the test verifies that only failing evaluators appear in the error message, you can simplify it.

Apply this diff:

-        assert "pii-detector" not in error_message or "pii-detector requires:" not in error_message
+        assert "pii-detector requires:" not in error_message

This directly checks that the passing evaluator doesn't appear in the "requires:" format, which is clearer and achieves the same goal.

---

180-200: Simplify the assertion logic for clarity.

Similar to Line 120, the assertion on Line 200 can be simplified for better readability.

Apply this diff:

-        assert "pii-detector" not in error_message or "pii-detector requires:" not in error_message
+        assert "pii-detector requires:" not in error_message

This directly verifies that the passing evaluator doesn't appear in the error message's "requires:" format.

packages/traceloop-sdk/tests/experiment/test_experiment.py (1)

268-314: Consider checking or prefixing the unused results variable.

The test focuses on validation errors, which is fine, but Line 301 assigns results without using it. For clarity and to silence the linter, either:

• Check that results is empty: assert len(results) == 0
• Prefix with underscore: _results, errors = ...

Apply this diff:

-        results, errors = await experiment._run_locally(
+        _results, errors = await experiment._run_locally(

Note: The unused row parameter on Line 283 is intentional for signature compatibility.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.