docs: API deprecate returning config for bulk endpoints #8732
Conversation
✅ Deploy Preview for determined-ui canceled.
|
| @@ -132,6 +132,7 @@ message Experiment { | |||
| // workspace). | |||
| bool parent_archived = 25; | |||
| // The configuration of the experiment. | |||
| // Will soon not be returned in list endpoints. | |||
There was a problem hiding this comment.
there is a deprecated annotation we'd probably want to use [deprecated = true];
There was a problem hiding this comment.
I think I misunderstood the change that added it initially, I assumed that config was just a nullable field that we started setting but now realize we did change the proto definitions. What do you think about basically just undoing this change?
3358e90#diff-0614f7e081e8db8d04bb236499cb0e1440bce3f5920e1c2b6a12aee860c28133R39
So add [deprecated = true] to experiments child and then add config back to GetExperimentResponse, I'll update the release note / proto deprecate notice if you think that change makes sense
There was a problem hiding this comment.
made those changes in this PR, let me know if you think it looks good
There was a problem hiding this comment.
It would achieve what the PR set out to do and you've probably checked these to double check:
what would our internal clients prefer? maybe what you were thinking in terms of having it be nullable and set on demand would make sense. A full revert of that would seem also add an endpoint that @mapmeld removed there. Do we have other endpoints to let the clients pull these configs in bulk?
There was a problem hiding this comment.
Yeah I didn't mean actually reverting the PR just undoing the config change that moved config from GetExperimentResponse to the proto experiment
No I don't think we have other endpoints that allow clients to do this currently but what I will say is that no one has used the config field in the year and a half for a valid use case. We use it for searcher name / type but we return this as a column on the experiment.
So I think removing then if someone has a valid use case making an optional parameter that sets it or making a different endpoint to support this would be the right call then.
On having it nullable and set on demand specifically I think this hard to do in a "type safe" way to do with proto. I'm not opposed to it but I think the change as is more clear from a type perspective.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8732 +/- ##
=======================================
Coverage 47.25% 47.26%
=======================================
Files 1045 1045
Lines 166697 166711 +14
Branches 2244 2243 -1
=======================================
+ Hits 78777 78792 +15
+ Misses 87762 87761 -1
Partials 158 158
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
is this the commit doing the revert e3d96ac ? |
Yeah it isn't a literal revert but I would just review the changes as this PR and not look per commit let me know if you think it looks good |
Description
Deprecate experiment config for bulk endpoints for performance reasons.
Remove this in a future version.
Test Plan
No testing needed
Commentary (optional)
Checklist
docs/release-notes/.See Release Note for details.
Ticket