fix(sdk): use direct workspace-scoped URL for experiment links - #7587
Closed
zhoufengen wants to merge 2 commits into
Closed
fix(sdk): use direct workspace-scoped URL for experiment links#7587zhoufengen wants to merge 2 commits into
zhoufengen wants to merge 2 commits into
Conversation
evaluate() and related entry points logged an opaque session-redirect URL (v1/session/redirect/experiments/...) for experiment results. This breaks in offline/air-gapped environments and does not match the URL shown in the browser address bar. Mirror the dataset URL fix (comet-ml#7440): get_experiment_url_by_id now builds the direct link opik/{workspace}/experiments/{dataset_id}/compare?experiments=[...] and its 5 call sites pass base_url and workspace (via client._dereferenced_workspace()). Adds parametrized unit tests covering both base_url shapes. Closes comet-ml#7462
Hosted via raw.githubusercontent.com so it renders inline in the PR body.
github-actions
Bot
added
python
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Comment on lines
605
to
610
| experiment_url = url_helpers.get_experiment_url_by_id( | ||
| experiment_id=experiment.id, | ||
| dataset_id=dataset.id, | ||
| url_override=client.config.url_override, | ||
| base_url=client.config.url_override, | ||
| workspace=client._dereferenced_workspace(), | ||
| ) |
Contributor
There was a problem hiding this comment.
Link rendering aborts evaluation
_evaluate_task() resolves experiment_url through client._dereferenced_workspace() without guarding the workspace lookup, so a failed GET /v1/private/auth/workspace can raise after scoring succeeds and prevent client.flush() and experiment.log_experiment_scores(...) from running — should we catch that lookup or defer URL construction?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/evaluation/evaluator.py around lines 605-610 inside
`_evaluate_task()`, `experiment_url = url_helpers.get_experiment_url_by_id(...)`
currently calls `workspace=client._dereferenced_workspace()` without protection, which
can trigger a real network GET for the workspace and raise after scoring has already
succeeded. Refactor this block to be best-effort: wrap the URL construction and
`report.display_experiment_link(...)` in a try/except that logs the failure and falls
back to a URL built without the workspace (or with a safe default), so the function
still reaches `client.flush()`,
`_try_notifying_about_experiment_completion(experiment)`, and
`experiment.log_experiment_scores(...)` and returns `evaluation_result_`. If possible,
ensure `client.flush()` and score logging occur in a finally/guaranteed path independent
of URL lookup errors.
Comment on lines
31
to
+36
| def get_experiment_url_by_id( | ||
| dataset_id: str, experiment_id: str, url_override: str | ||
| dataset_id: str, experiment_id: str, base_url: str, workspace: str | ||
| ) -> str: | ||
| encoded_opik_url = base64.b64encode(url_override.encode("utf-8")).decode("utf-8") | ||
|
|
||
| project_path = urllib.parse.quote( | ||
| f"v1/session/redirect/experiments/?experiment_id={experiment_id}&dataset_id={dataset_id}&path={encoded_opik_url}", | ||
| safe=ALLOWED_URL_CHARACTERS, | ||
| ) | ||
| return urllib.parse.urljoin(ensure_ending_slash(url_override), project_path) | ||
| domain_root = get_base_url(base_url) | ||
| experiments_param = urllib.parse.quote(json.dumps([experiment_id])) | ||
| return f"{domain_root}opik/{workspace}/experiments/{dataset_id}/compare?experiments={experiments_param}" |
Contributor
There was a problem hiding this comment.
Undocumented URL contract
get_experiment_url_by_id is still undocumented, so callers have to infer that base_url is normalized, workspace is embedded in the path, and experiment_id becomes ?experiments=[...], which makes the new URL contract easy to miss — should we add a short docstring here?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/src/opik/url_helpers.py around lines 31-36, update the
`get_experiment_url_by_id` function by adding a short docstring that documents the new
public contract. Explicitly state that `base_url` is normalized via `get_base_url`, that
`workspace` is embedded in the URL path, and that the `experiments` query parameter is a
JSON array containing only `experiment_id` (i.e., `experiments=["<id>"]`) and then
URL-encoded. Keep it brief but concrete so downstream callers know exactly what the
helper returns and what each argument affects.
Contributor
|
Thanks so much for taking the time to put this together — really appreciate the effort! We ended up merging #7593 which addresses the same issue, so I'm going to close this one out to avoid duplicate work. Feel free to open a PR for anything else you spot — contributions like this are always welcome! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
evaluate()and related entry points printed an opaque session-redirect URL (v1/session/redirect/experiments/?...) for experiment results. That URL is hard to copy into docs/notebooks/CI logs and breaks entirely in offline or air-gapped environments where the Opik server is unreachable.This change mirrors the dataset URL fix (#7440): experiment links now point directly at the frontend compare page, matching the URL shown in the browser address bar.
Changes
url_helpers.get_experiment_url_by_idnow builds the direct linkopik/{workspace}/experiments/{dataset_id}/compare?experiments=[...](JSON-arrayexperimentsquery param, the format the compare page reads) instead of the legacy session-redirect path.(dataset_id, experiment_id, url_override)to(dataset_id, experiment_id, base_url, workspace).evaluator.pyto passbase_url=client.config.url_overrideandworkspace=client._dereferenced_workspace().Testing
tests/unit/test_url_helpers.pycovering bothbase_urlshapes (local and comet.com).test_url_helpers.pypasses: 15 passed (including the 2 new experiment-URL cases).ruff check,ruff format --check, andmypyon the changed files.Test suite passing on the fix branch:
Related Issues
Closes #7462