Add experiment link in the SDK #353
Conversation
| dataset_id = dataset.id | ||
|
|
||
| config = opik.config.OpikConfig() | ||
| ui_url = get_ui_url() |
There was a problem hiding this comment.
When this method is called, OpikConfig will be reinitialized again, which can be considered a resource-intensive operation as it involves reading environment variables, config file, etc.
There was a problem hiding this comment.
@japdubengsub The evaluate function is already very resource intensive and takes at least 15 seconds to run, don't think this will be an issue. Is there another way to get the configuration that is already read ? If not, we can keep as is
|
|
||
|
|
||
| def get_ui_url() -> str: | ||
| config = opik.config.OpikConfig() | ||
| opik_url_override = config.url_override | ||
|
|
||
| return opik_url_override.rstrip("/api") | ||
|
|
||
|
|
||
| def get_experiment_url(dataset_name: str, experiment_id: str) -> str: |
There was a problem hiding this comment.
The whole Experiment object could have been passed here since it contains all the necessary fields – dataset_name, client, and experiment ID.
This method could even be made a method of the "Experiment" class itself.
There was a problem hiding this comment.
@japdubengsub Works as is, we can refactor this in the future if we keep it as is
| scoring_metrics=[metrics.Equals()], | ||
| task_threads=1, | ||
| ) | ||
| with mock.patch.object( |
There was a problem hiding this comment.
Just to point out, the test does not verify whether a notification was actually printed.
There was a problem hiding this comment.
I didn't know how to add tests for it but I manually verified it works, we can add a test in the future if it fails
There was a problem hiding this comment.
we can add this to our e2e flow later, when we do sdk check we fetch URL and open it + test data is there
@AndreiCautisanu FYI please add to backlog
Details
Display a link to the experiment when the experiment ends, the URL requires the dataset ID which is not known which leads to 1 extra API call but shouldn't degrade the experience too much. The output from an experiment is now:
In a follow up PR we can consider removing the
Uploading results to Opik...message as it is now redundant (assuming the upload time is relatively small)