[issue-7462] [SDK] fix: use direct workspace-scoped URL for experiment link in evaluate() - #7589
Closed
Aftabbs wants to merge 1 commit into
Closed
[issue-7462] [SDK] fix: use direct workspace-scoped URL for experiment link in evaluate()#7589Aftabbs wants to merge 1 commit into
Aftabbs wants to merge 1 commit into
Conversation
…t link in evaluate() Replaces the legacy session-redirect URL with the same direct URL pattern used by get_dataset_url_by_id and get_test_suite_url_by_id. The session-redirect URL surfaces the experiment under the wrong scope; the direct URL opens the compare page immediately in the correct workspace. Closes comet-ml#7462
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
+609
| 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._workspace, |
Contributor
There was a problem hiding this comment.
Placeholder workspace builds wrong URLs
_evaluate_task() and the other evaluator link sites pass workspace=client._workspace into url_helpers.get_experiment_url_by_id(), so when OpikConfig.workspace is the "default" placeholder the compare link points at /opik/default/... instead of the resolved workspace — should we route this through Opik._dereferenced_workspace() first?
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 549-630 inside
`_evaluate_task`, the `experiment_url = url_helpers.get_experiment_url_by_id(...)` call
passes `workspace=client._workspace`, which embeds the raw workspace placeholder into
the URL. Refactor it to pass the dereferenced/resolved workspace (e.g.,
`workspace=client._dereferenced_workspace()` or an equivalent public accessor) so when
the configured workspace is "default" the generated link targets the actual workspace.
Apply the same fix to the other `url_helpers.get_experiment_url_by_id()` occurrences in
this file: `__internal_api__run_test_suite__` (~lines 408-415), `evaluate_prompt`
(~1192-1199), `evaluate_experiment` (~886-892), and `_evaluate_test_suite_task`
(~739-745).
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 public URL contract
get_experiment_url_by_id() now uses workspace to build the compare-page link, but the public contract isn't documented, so callers can't tell that workspace is required or what URL shape they get — can we add a short docstring covering that?
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 public function
`get_experiment_url_by_id` (new signature with `dataset_id`, `experiment_id`,
`base_url`, `workspace`) to include a short docstring. Explain that `workspace` is
required to scope the returned link and that the function returns the direct experiments
compare page URL in the form
`...opik/{workspace}/experiments/{dataset_id}/compare?experiments=...`. Keep it
consistent with the surrounding URL helper documentation so callers can rely on the
documented contract.
Contributor
|
Thanks for the PR! We just merged #7593 which fixes this same issue, so I'll close this one to avoid overlap. Appreciate you taking the time to dig into it - hope to see more contributions from you! |
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.
Details
get_experiment_url_by_idwas generating a session-redirect URL(
v1/session/redirect/experiments/?experiment_id=...&dataset_id=...) thatroutes through the user's session context. This surfaced the experiment under
the wrong scope in the UI instead of opening the compare view directly.
The fix replaces the session-redirect URL with the direct compare-page URL
pattern used by
get_dataset_url_by_idandget_test_suite_url_by_id(addedin #7467):
The
workspaceis added as a new parameter (available asclient._workspaceat all call sites in
evaluator.py).Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
New test:
test_get_experiment_url_by_id__returns_direct_workspace_scoped_url— 3 parametrized cases (localhost, localhost with trailing slash, cloud URL).
Run with:
Documentation
No documentation change required — this fixes an internal log/link URL, not a public API.