Skip to content

fix(mlflow): get() fetches spans via 3.x API; materialize() - #152

Merged
julpayne merged 11 commits into
eval-hub:mainfrom
suppathak:fix/mlflow-get-includes-spans
Sep 15, 2026
Merged

julpayne merged 11 commits into
eval-hub:mainfrom
suppathak:fix/mlflow-get-includes-spans

Conversation

@suppathak

@suppathak suppathak commented Jul 6, 2026 •

Copy link
Copy Markdown
Contributor

What and why

Minor follow-up to #144 . After the 3.x compatibility update, get() still called GET /api/2.0/mlflow/traces/{id}/info which returns only trace metadata — no span data. As a result, materialize() produced files with "data": {} — empty spans — causing downstream consumers (e.g. the CLEAR adapter) to see zero LLM calls and skip all traces.

Verified against a local MLflow 3.13.0 server end-to-end.

Type

  • fix

Changes

  • Add _API_V3 = "/api/3.0/mlflow" constant
  • Add _get_v3() on MlflowClient using the 3.x base path
  • Update TracesNamespace.get() to call GET /api/3.0/mlflow/traces/get?trace_id=... — this endpoint returns the full trace with spans
  • Update materialize() to call get() per trace so each written .json file includes data.spans

Testing

  • Tested manually against local MLflow 3.13.0 — materialize() now writes files with spans included

Breaking changes

None — get() signature is unchanged, behavior is now correct.

Summary by CodeRabbit

  • New Features
    • Added support for creating MLflow traces and uploading trace files.
    • Added compatibility with MLflow 3.x trace APIs.
    • Trace exports now preserve workspace-scoped artifact paths.
  • Bug Fixes
    • Improved MLflow trace retrieval for newer releases.
    • Exports now refresh traces to include complete span details and related fields.
    • Added resilient timestamp and duration normalization.
    • Exports continue when individual trace refreshes fail.
  • Tests
    • Updated coverage for MLflow 3.x trace parsing and materialization.

@coderabbitai

coderabbitai Bot commented Jul 6, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 380956a8-d73d-4bc0-a635-f60d3410464a

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The MLflow adapter adds MLflow 3 trace creation and file upload workflows, normalizes trace data, preserves workspace-scoped artifact paths, and concurrently materializes complete trace payloads. Unit tests cover the updated parsing and materialization responses.

Changes

MLflow 3 trace creation and upload

Layer / File(s) Summary
Trace creation and file upload
src/evalhub/adapter/mlflow.py
Adds OTLP span upload, MLflow 3 trace registration, public trace creation, and trace-file upload with span ID, status, and timestamp normalization.

Trace retrieval and materialization

Layer / File(s) Summary
Trace retrieval and span normalization
src/evalhub/adapter/mlflow.py, tests/unit/test_mlflow_traces.py
Retrieves traces through /api/3.0/mlflow/traces/get and normalizes timing values and protobuf-style span attributes into parsed fields.
Artifact paths and complete materialization
src/evalhub/adapter/mlflow.py, tests/unit/test_mlflow_traces.py
Preserves workspace-scoped artifact paths, adds artifact upload logging, re-fetches traces concurrently, and writes complete parsed trace data while skipping failed fetches.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TraceFile
  participant TracesNamespace
  participant OTLPTraceAPI
  participant MLflow3TraceAPI
  TraceFile->>TracesNamespace: upload_trace_file(experiment_id, trace_path)
  TracesNamespace->>TracesNamespace: normalize spans and timestamps
  TracesNamespace->>OTLPTraceAPI: POST /v1/traces
  OTLPTraceAPI-->>TracesNamespace: upload response
  TracesNamespace->>MLflow3TraceAPI: POST /api/3.0/mlflow/traces
  MLflow3TraceAPI-->>TracesNamespace: trace_id
Loading

Possibly related PRs

Suggested labels: community-pr

Suggested reviewers: julpayne, ruivieira

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the MLflow 3.x span retrieval and materialization changes.
Description check ✅ Passed The description explains the issue, changes, testing, type, and breaking-change status, but omits the required issue-closing line and test checkbox.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@suppathak

Copy link
Copy Markdown
Contributor Author

cc @hemajv

@julpayne julpayne left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@suppathak this looks good - thanks - I did have a question though, do you know in which mlflow version this v3 API was introduced so that we can provide some compatability guidance?

@suppathak

Copy link
Copy Markdown
Contributor Author

@suppathak this looks good - thanks - I did have a question though, do you know in which mlflow version this v3 API was introduced so that we can provide some compatability guidance?

@julpayne . Thanks for checking. TThe /api/3.0/mlflow trace endpoints were introduced in MLflow 3.0 (released 2025). Trace fetching with spans requires MLflow server ≥ 3.0. MLflow 2.x servers do not expose these endpoints.

@suppathak
suppathak requested a review from julpayne July 8, 2026 22:31
@suppathak

Copy link
Copy Markdown
Contributor Author

cc @ruivieira

@julpayne

Copy link
Copy Markdown
Collaborator

@suppathak can you check the failing tests please?

@suppathak

Copy link
Copy Markdown
Contributor Author

@julpayne , Please check! Thanks

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/evalhub/adapter/mlflow.py (1)

790-793: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider cleaning up the unused experiment_id parameter.

The experiment_id parameter is no longer used by the v3 trace retrieval endpoint. If this method must maintain its existing signature for backwards compatibility, consider prefixing the parameter with an underscore (e.g., _experiment_id: str) or adding a brief docstring note. If not, it can be safely removed.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/evalhub/adapter/mlflow.py` around lines 790 - 793, Clean up the unused
experiment_id parameter in the trace-fetching method surrounding
_client._get_v3("/traces/get"). Remove it if the method’s API permits; otherwise
retain the signature for compatibility by renaming it to _experiment_id and
briefly documenting that it is unused by the v3 endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/evalhub/adapter/mlflow.py`:
- Around line 862-882: Update the trace materialization loop around self.get and
file writing to use a bounded ThreadPoolExecutor, fetching and processing full
traces concurrently while preserving the existing trace filename sanitization
and JSON structure. Submit one task per trace, collect task results so
exceptions still propagate, and avoid unbounded concurrency against the tracking
server.

---

Nitpick comments:
In `@src/evalhub/adapter/mlflow.py`:
- Around line 790-793: Clean up the unused experiment_id parameter in the
trace-fetching method surrounding _client._get_v3("/traces/get"). Remove it if
the method’s API permits; otherwise retain the signature for compatibility by
renaming it to _experiment_id and briefly documenting that it is unused by the
v3 endpoint.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d9543a24-d0a3-429f-8598-acbf41cdc338

📥 Commits

Reviewing files that changed from the base of the PR and between 23c81f7 and f1572ac.

📒 Files selected for processing (2)
  • src/evalhub/adapter/mlflow.py
  • tests/unit/test_mlflow_traces.py

Comment thread src/evalhub/adapter/mlflow.py Outdated

@julpayne julpayne left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@julpayne
julpayne requested a review from ruivieira July 15, 2026 13:14

@ruivieira ruivieira left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @suppathak , LGTM, just add one question (inlined).

Also, is it possible to rebase this on main? Thank you!

collected += 1
if collected >= max_results:
break
for future in as_completed(future_to_id):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@suppathak Should we wrap future.result() in a try/except so a single unreachable or 404'd trace doesn't abort the entire batch?
With the old loop, a failed trace skipped that entry and the rest of the batch continued.
So instead of

for future in as_completed(future_to_id):
    full_trace = future.result()  # raises on any failure

we could have

for future in as_completed(future_to_id):
    try:
        full_trace = future.result()
    except Exception:
        rid = future_to_id[future]
        logger.warning("Failed to fetch trace %s, skipping", rid, exc_info=True)
        continue

wdyt?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Thanks @ruivieira .

I have addressed both your comments. Ptal.

@suppathak
suppathak force-pushed the fix/mlflow-get-includes-spans branch from fd81eaa to a164fa0 Compare July 15, 2026 16:34
@suppathak
suppathak requested a review from ruivieira July 15, 2026 16:35

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/evalhub/adapter/mlflow.py`:
- Around line 791-796: Update the v3 trace retrieval flow around _parse_trace to
normalize trace_info.request_time into the parsed trace start timestamp and
execution_duration into its duration before materializing the trace, while
preserving existing field handling where applicable. Adjust the trace tests to
use the documented {"trace": {"trace_info": ..., "spans": [...]}} response shape
and verify the normalized timing values.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e5e9d7b1-f3cc-49f4-8054-de1adfdd5061

📥 Commits

Reviewing files that changed from the base of the PR and between f1572ac and a164fa0.

📒 Files selected for processing (2)
  • src/evalhub/adapter/mlflow.py
  • tests/unit/test_mlflow_traces.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/unit/test_mlflow_traces.py

Comment thread src/evalhub/adapter/mlflow.py Outdated
@suppathak
suppathak requested a review from julpayne July 20, 2026 21:04
@suppathak
suppathak force-pushed the fix/mlflow-get-includes-spans branch from 4ae6ea6 to 9f522a5 Compare July 27, 2026 19:42

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/unit/test_mlflow_traces.py`:
- Around line 329-331: Update the mock_client._get_v3 assertion in the test
around TracesNamespace.get() to verify calls include the exact path
"/traces/get" together with each trace_id parameter. Match calls without relying
on ordering, since trace materialization is concurrent, while retaining the
existing response side effect.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1ab658ee-0f9b-42e5-9d3b-b7bce599be68

📥 Commits

Reviewing files that changed from the base of the PR and between 4ae6ea6 and 9f522a5.

📒 Files selected for processing (2)
  • src/evalhub/adapter/mlflow.py
  • tests/unit/test_mlflow_traces.py

Comment on lines +329 to +331
mock_client._get_v3 = MagicMock(
side_effect=lambda path, params: _get_v3_responses[params["trace_id"]]
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the v3 endpoint path.

The side effect ignores path, so this test passes even if TracesNamespace.get() calls the wrong v3 endpoint. Assert calls include ("/traces/get", {"trace_id": ...}); use order-independent matching because materialization is concurrent.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_mlflow_traces.py` around lines 329 - 331, Update the
mock_client._get_v3 assertion in the test around TracesNamespace.get() to verify
calls include the exact path "/traces/get" together with each trace_id
parameter. Match calls without relying on ordering, since trace materialization
is concurrent, while retaining the existing response side effect.

@suppathak

suppathak commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor Author

@ruivieira @julpayne

Latest update

Added in this push:

  1. Artifact path fix — _artifact_server_path now preserves the workspaces/{name}/ prefix when present in artifact_uri. Required when bypassing the EvalHub sidecar on OpenShift AI (the sidecar normally injects this prefix, but direct calls to the MLflow service need it explicitly).

  2. TracesNamespace.create() — Upload spans to MLflow via OTLP protobuf (POST /v1/traces) + register trace metadata via v3 API (POST /api/3.0/mlflow/traces). Mirrors the official MLflow SDK's two-step upload behavior.

  3. TracesNamespace.upload_trace_file() — Convenience method to upload trace JSON files (as exported by materialize() or MLflow UI).

  4. HTTP helpers — _post_v3, _put_json, _patch for v3/v2 API calls.

  5. Debug logging in _put_artifact for troubleshooting artifact uploads.

Downstream PR: eval-hub/eval-hub-contrib#104

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/evalhub/adapter/mlflow.py (1)

587-596: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Do not inject workspaces/default when MLFLOW_WORKSPACE is unset.

The fallback now always adds a workspaces/{workspace} segment. When MLFLOW_WORKSPACE is unset, the code uses the literal default workspace. Upstream MLflow 3.x has no workspace concept, so the PUT then targets a path that the artifact store does not expect. The docstring at Line 571-573 still documents the unscoped fallback {experiment_id}/{run_id}/artifacts/{path}, which no longer matches the code.

Apply the workspace prefix only when the environment variable is set, and align the docstring.

🛠️ Proposed fix
         if experiment_id and run_id:
-            workspace = os.environ.get("MLFLOW_WORKSPACE", "default")
-            return (
-                f"{_ARTIFACTS_API}/workspaces/{workspace}/{experiment_id}/{run_id}/artifacts/{artifact_path}"
-            )
+            workspace = os.environ.get("MLFLOW_WORKSPACE")
+            prefix = f"workspaces/{workspace}/" if workspace else ""
+            return (
+                f"{_ARTIFACTS_API}/{prefix}"
+                f"{experiment_id}/{run_id}/artifacts/{artifact_path}"
+            )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/evalhub/adapter/mlflow.py` around lines 587 - 596, Update the artifact
upload path resolution around the experiment_id/run_id fallback to add the
workspaces/{workspace} segment only when MLFLOW_WORKSPACE is explicitly set;
otherwise return the unscoped {experiment_id}/{run_id}/artifacts/{artifact_path}
path. Align the method’s docstring with both resulting formats and remove the
default workspace fallback.
🧹 Nitpick comments (4)
src/evalhub/adapter/mlflow.py (4)

1086-1096: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse _to_ms for the timing fields.

_to_ms already converts integer ms, ISO-8601 strings, and duration strings. This block repeats the ISO parsing and misses execution_duration.

♻️ Proposed refactor
-        exec_ms = info.get("execution_time_ms") or info.get("execution_duration_ms")
-        ts_ms = info.get("timestamp_ms")
-        if not ts_ms:
-            req_time = info.get("request_time")
-            if isinstance(req_time, str):
-                import datetime
-                try:
-                    dt = datetime.datetime.fromisoformat(req_time.replace("Z", "+00:00"))
-                    ts_ms = int(dt.timestamp() * 1000)
-                except (ValueError, TypeError):
-                    ts_ms = None
+        exec_ms = _to_ms(
+            info.get("execution_time_ms")
+            or info.get("execution_duration_ms")
+            or info.get("execution_duration")
+        )
+        ts_ms = _to_ms(info.get("timestamp_ms") or info.get("request_time"))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/evalhub/adapter/mlflow.py` around lines 1086 - 1096, Update the
timing-field conversion in the surrounding evaluation logic to reuse the
existing _to_ms helper for request_time, execution_time_ms,
execution_duration_ms, and timestamp_ms. Replace the duplicated ISO parsing and
ensure execution_duration is included while preserving the existing fallback
precedence and millisecond values.

1228-1230: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Cancel remaining futures when max_results is reached.

The break exits the as_completed loop, but ThreadPoolExecutor.__exit__ calls shutdown(wait=True), so all pending fetches still run. Cancel them to avoid unnecessary calls to the tracking server.

♻️ Proposed refactor
                     collected += 1
                     if collected >= max_results:
+                        for pending in future_to_id:
+                            pending.cancel()
                         break
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/evalhub/adapter/mlflow.py` around lines 1228 - 1230, When the max_results
threshold is reached in the as_completed loop, cancel all still-pending futures
before breaking so ThreadPoolExecutor cleanup does not wait for unnecessary
tracking-server fetches. Update the surrounding future collection logic to
identify unfinished futures and invoke their cancellation, while preserving the
existing collected results and break behavior.

942-945: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Function-local imports repeat across the new trace code. Four new call paths import modules inside function bodies. Move the standard-library imports to module scope. Keep only the optional third-party import local, and then document why.

  • src/evalhub/adapter/mlflow.py#L942-L945: keep the opentelemetry.proto imports local only if the dependency is optional, and add a comment that states this.
  • src/evalhub/adapter/mlflow.py#L705-L705: move datetime to the module imports.
  • src/evalhub/adapter/mlflow.py#L1062-L1062: move base64 to the module imports.
  • src/evalhub/adapter/mlflow.py#L1091-L1091: remove this datetime import; the module-level import covers it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/evalhub/adapter/mlflow.py` around lines 942 - 945, Move the
standard-library imports used by the new trace paths into module scope: add
datetime for the code around line 705 and base64 for the code around line 1062,
then remove the redundant datetime import around line 1091. In the trace code
around line 942, keep the opentelemetry imports local only if that dependency is
optional, and add a comment documenting that rationale. Apply these changes in
src/evalhub/adapter/mlflow.py at lines 942-945, 705, 1062, and 1091.

340-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the unused _put_json and _patch helpers.

These methods only define client methods and have no callers in the tracked Python code. Drop them to keep the MLflow client surface minimal.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/evalhub/adapter/mlflow.py` around lines 340 - 349, Remove the unused
_put_json and _patch methods from the MLflow client class, including their URL
construction, HTTP calls, and response handling, while leaving the remaining
client methods unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/evalhub/adapter/mlflow.py`:
- Around line 1007-1015: Update src/evalhub/adapter/mlflow.py at lines 1007-1015
to declare root_inputs and root_outputs as str initialized to "{}"; annotate
proto_spans at lines 959-959 as list[trace_pb2.Span] and span_dicts at lines
1098-1098 as list[dict[str, Any]], ensuring the surrounding MLflow
span-processing code passes mypy strict mode and supports string slicing.
- Around line 969-980: Update the span construction in create() to use each
span’s computed status_raw value instead of always assigning
trace_pb2.Status.STATUS_CODE_OK. Map the documented per-span status to the
corresponding trace_pb2.Status code while preserving OK as the default when no
status is provided.
- Around line 745-754: Update the span attribute normalization block around
raw_attrs and normalized: treat a null or otherwise absent non-list attributes
value as an empty mapping before calling dict, while preserving existing list
handling. Add an explicit type annotation to normalized using the appropriate
span/result type so mypy strict mode can infer its contents.
- Around line 1075-1084: Update _decode_span_id to recognize exactly 16
hexadecimal characters case-insensitively and return the normalized hex value.
Decode other inputs with base64 validation enabled, accept the result only when
it is exactly 8 bytes (returning its 16-character hex form), and preserve the
existing fallback behavior for invalid or incorrectly sized values.

---

Outside diff comments:
In `@src/evalhub/adapter/mlflow.py`:
- Around line 587-596: Update the artifact upload path resolution around the
experiment_id/run_id fallback to add the workspaces/{workspace} segment only
when MLFLOW_WORKSPACE is explicitly set; otherwise return the unscoped
{experiment_id}/{run_id}/artifacts/{artifact_path} path. Align the method’s
docstring with both resulting formats and remove the default workspace fallback.

---

Nitpick comments:
In `@src/evalhub/adapter/mlflow.py`:
- Around line 1086-1096: Update the timing-field conversion in the surrounding
evaluation logic to reuse the existing _to_ms helper for request_time,
execution_time_ms, execution_duration_ms, and timestamp_ms. Replace the
duplicated ISO parsing and ensure execution_duration is included while
preserving the existing fallback precedence and millisecond values.
- Around line 1228-1230: When the max_results threshold is reached in the
as_completed loop, cancel all still-pending futures before breaking so
ThreadPoolExecutor cleanup does not wait for unnecessary tracking-server
fetches. Update the surrounding future collection logic to identify unfinished
futures and invoke their cancellation, while preserving the existing collected
results and break behavior.
- Around line 942-945: Move the standard-library imports used by the new trace
paths into module scope: add datetime for the code around line 705 and base64
for the code around line 1062, then remove the redundant datetime import around
line 1091. In the trace code around line 942, keep the opentelemetry imports
local only if that dependency is optional, and add a comment documenting that
rationale. Apply these changes in src/evalhub/adapter/mlflow.py at lines
942-945, 705, 1062, and 1091.
- Around line 340-349: Remove the unused _put_json and _patch methods from the
MLflow client class, including their URL construction, HTTP calls, and response
handling, while leaving the remaining client methods unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 960a0af3-9a90-4cd3-aee2-d2ec0efa6f95

📥 Commits

Reviewing files that changed from the base of the PR and between 9f522a5 and e880a37.

📒 Files selected for processing (2)
  • src/evalhub/adapter/mlflow.py
  • tests/unit/test_mlflow_traces.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/unit/test_mlflow_traces.py

Comment thread src/evalhub/adapter/mlflow.py Outdated
Comment thread src/evalhub/adapter/mlflow.py
Comment thread src/evalhub/adapter/mlflow.py Outdated
Comment on lines +1007 to +1015
root_inputs = {}
root_outputs = {}
for span in spans:
if not span.get("parent_span_id"):
attrs = span.get("attributes", {})
ri = attrs.get("mlflow.spanInputs", "{}")
ro = attrs.get("mlflow.spanOutputs", "{}")
root_inputs = ri if isinstance(ri, str) else json.dumps(ri, default=str)
root_outputs = ro if isinstance(ro, str) else json.dumps(ro, default=str)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Missing and conflicting type annotations break mypy strict mode. The new code assigns empty collections without annotations, and root_inputs/root_outputs start as dict and are reassigned to str. mypy strict reports Need type annotation and Incompatible types in assignment for these sites, and root_inputs[:100] at Line 1040 is invalid for a dict.

  • src/evalhub/adapter/mlflow.py#L1007-L1015: declare root_inputs: str = "{}" and root_outputs: str = "{}", so the JSON string type is consistent and the slicing at Line 1040-1041 type-checks.
  • src/evalhub/adapter/mlflow.py#L959-L959: annotate proto_spans: list[trace_pb2.Span] = [].
  • src/evalhub/adapter/mlflow.py#L1098-L1098: annotate span_dicts: list[dict[str, Any]] = [].

As per coding guidelines "Pass mypy strict-mode type checking, targeting Python 3.11".

🧰 Tools
🪛 ast-grep (0.45.0)

[info] 1013-1013: use jsonify instead of json.dumps for JSON output
Context: json.dumps(ri, default=str)
Note: [CWE-116] Improper Encoding or Escaping of Output.

(use-jsonify)


[info] 1014-1014: use jsonify instead of json.dumps for JSON output
Context: json.dumps(ro, default=str)
Note: [CWE-116] Improper Encoding or Escaping of Output.

(use-jsonify)

📍 Affects 1 file
  • src/evalhub/adapter/mlflow.py#L1007-L1015 (this comment)
  • src/evalhub/adapter/mlflow.py#L959-L959
  • src/evalhub/adapter/mlflow.py#L1098-L1098
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/evalhub/adapter/mlflow.py` around lines 1007 - 1015, Update
src/evalhub/adapter/mlflow.py at lines 1007-1015 to declare root_inputs and
root_outputs as str initialized to "{}"; annotate proto_spans at lines 959-959
as list[trace_pb2.Span] and span_dicts at lines 1098-1098 as list[dict[str,
Any]], ensuring the surrounding MLflow span-processing code passes mypy strict
mode and supports string slicing.

Source: Coding guidelines

Comment thread src/evalhub/adapter/mlflow.py Outdated
@suppathak
suppathak force-pushed the fix/mlflow-get-includes-spans branch from e880a37 to 5f6a847 Compare August 3, 2026 23:32
@julpayne

Copy link
Copy Markdown
Collaborator

@suppathak can you run pre-commit run --all-files so that the checks pass please?

@suppathak

Copy link
Copy Markdown
Contributor Author

@julpayne, apologies for the delayed update. I've run pre-commit run --all-files and pushed the fixes — mostly formatting (ruff-format) and added type: ignore for the opentelemetry protobuf imports that lack stubs. Also updated two artifact path tests to match the workspace-preserving behavior. The only remaining mypy error is a pre-existing one in cli/formatter.py (unrelated to this PR).

Comment thread src/evalhub/adapter/mlflow.py Outdated
f"{_ARTIFACTS_API}/{experiment_id}/{run_id}/artifacts/{artifact_path}"
)
workspace = os.environ.get("MLFLOW_WORKSPACE", "default")
return f"{_ARTIFACTS_API}/workspaces/{workspace}/{experiment_id}/{run_id}/artifacts/{artifact_path}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@suppathak this unconditionally adds workspaces/default/ even when the workspace is not defined. Should we have a fallback for when it's not defined? wdyt?

@julpayne

julpayne commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator

Other Improvements Needed

Beyond that last comment, there are several other issues in this PR worth addressing:

  1. Unused _put_json and _patch methods (Medium)
    The PR adds _put_json() and _patch() helper methods on MlflowClient (around lines 340-349 in the diff), but neither has any callers in the codebase. Dead code should be removed or deferred to the PR that actually needs it.

  2. Unused experiment_id parameter in get() (Low)
    After switching to the v3 API, TracesNamespace.get() no longer passes experiment_id to the endpoint — the v3 GET /traces/get only uses trace_id. The parameter remains in the signature but is silently ignored. Either remove it (breaking change) or rename to _experiment_id with a deprecation note.

  3. Test doesn't assert the v3 endpoint path (Low-Medium)
    In the test for materialize(), the _get_v3 mock uses a side_effect that ignores the path argument:

    mock_client._get_v3 = MagicMock(
      side_effect=lambda path, params: _get_v3_responses[params["trace_id"]]
    )

    This means the test would still pass if get() called the wrong endpoint (e.g., /traces/wrong). An assertion on the call args should verify the exact path "/traces/get" was used.

  4. Cancel pending futures on max_results (Low)
    When collected >= max_results, the code breaks out of the as_completed loop but the ThreadPoolExecutor.exit still calls shutdown(wait=True), meaning all pending fetches complete unnecessarily. Adding future.cancel() for remaining futures would avoid wasted requests against the tracking server.

  5. Duplicated timing conversion logic (Low)
    The _to_ms() helper already handles ISO-8601 strings, duration strings, and plain ints. But later in the code (around lines 1086-1096 per CodeRabbit), there's duplicated ISO datetime parsing. This should reuse _to_ms() for consistency and DRY.

  6. Repeated function-local imports (Trivial)
    Standard-library imports like datetime and base64 are imported inside function bodies in multiple places. These should be at module scope. Only the opentelemetry.proto imports should remain local since that dependency is optional.

  7. PR needs rebase on main
    ruivieira requested a rebase back in July. The branch was force-pushed several times since but given the number of commits added in August and main being at 71ce82a, it likely needs another rebase before merge.

Summary: ruivieira's last comment is valid and should be addressed before merging. The workspace prefix issue (#1 above) is the most critical outstanding item. Items #2-3 are worth fixing in this PR; items #4-7 are nice-to-haves that could be deferred.

@julpayne

Copy link
Copy Markdown
Collaborator

@suppathak could you please rebase, fix the failing checks and look at the other proposed enhancements? Sorry to be so picky...

…spans

- Add _API_V3 = "/api/3.0/mlflow" constant and _get_v3() on MlflowClient
- Update TracesNamespace.get() to call GET /api/3.0/mlflow/traces/get?trace_id=...
  (previously used /api/2.0/mlflow/traces/{id}/info which returns only metadata)
- Update materialize() to call get() per trace so each written file includes
  full span data instead of an empty data: {} dict
…elds

MLflow v3 returns span attributes as protobuf-wrapped key-value pairs.
Downstream consumers like CLEAR expect top-level inputs, outputs, and
span_type fields. Add unwrap and normalize helpers in _parse_trace().
…reate()

- _artifact_server_path: keep workspaces/{name}/ prefix when present in
  artifact_uri (required when bypassing the EvalHub sidecar on OpenShift AI)
- Add _post_v3, _put_json, _patch HTTP helpers for v3/v2 API calls
- TracesNamespace.create(): upload spans via OTLP protobuf + register
  trace metadata via v3 API (mirrors official MLflow SDK behavior)
- TracesNamespace.upload_trace_file(): convenience method for uploading
  trace JSON files exported from MLflow UI or materialize()
- Add debug logging to _put_artifact for troubleshooting
@suppathak
suppathak force-pushed the fix/mlflow-get-includes-spans branch from 6296421 to 766a643 Compare September 15, 2026 15:52
@julpayne
julpayne merged commit 593ac9a into eval-hub:main Sep 15, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants