-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
exp push: push experiments revs to studio #9160
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #9160 +/- ##
==========================================
- Coverage 92.91% 92.65% -0.26%
==========================================
Files 456 457 +1
Lines 36884 36918 +34
Branches 5324 5329 +5
==========================================
- Hits 34269 34205 -64
- Misses 2091 2168 +77
- Partials 524 545 +21
... and 21 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@@ -15,10 +15,12 @@ | |||
|
|||
logger = logging.getLogger(__name__) | |||
|
|||
STUDIO_ENDPOINT = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently blocked on endpoint. I talked with @amritghimire about the API. It has to be a separate endpoint, and for the payload, it seems refs
are enough.
If Studio needs more info regarding experiment refs, we can iterate as we go, as this will be behind a feature flag till the endpoint is stable.
return [ref.name for ref in refs] | ||
|
||
|
||
def _notify_studio( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will move to dvc-studio-client
when we remove feature flag.
@dberenbaum @iterative/dvc, can you please review this? I know there is not much here and we don't have endpoint (endpoint is almost there), but I intend to merge this. Can we review code or workflow please? :) |
@@ -300,6 +300,7 @@ def __call__(self, data): | |||
"feature": FeatureSchema( | |||
{ | |||
Optional("machine", default=False): Bool, | |||
Optional("push_exp_to_studio", default=False): Bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the motivation for putting this under a feature flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Endpoint and data format may change. For example, we have yet to figure out if refs
will be enough for all git-forges to check if the refs exist with the API, before Studio even clones. We still don't know if bitbucket supports them for example.
Also, I don't want any users to hit any endpoint until this feature is considered stable.
The plan is to release this internally behind a flag, and see what the performance of Studio will be, what needs to be improved, etc to make it as close to live metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have to figure out workflow (eg: what will be the ways to enable/disable push to studio?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted a bit of flexibility since there are lots of questions regarding workflow/API, etc.
Currently blocked, as we don't have any endpoints. We have agreed on the payload for now.
This will be behind a feature flag before the endpoint is stable. You can turn this by following ways:
dvc config feature.push_exp_to_studio true
To push experiments, it needs a
STUDIO_TOKEN
envvar set.And just do
dvc exp push
.I'll merge this on approval, as everything is behind a feature-flag, and requires
STUDIO_TOKEN
to be set.Closes #9163.