-
Notifications
You must be signed in to change notification settings - Fork 362
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
fix: assign only run in a single run experiment as best_trial_id #9051
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9051 +/- ##
==========================================
+ Coverage 53.57% 57.40% +3.83%
==========================================
Files 1255 761 -494
Lines 152855 102902 -49953
Branches 3298 3298
==========================================
- Hits 81885 59072 -22813
+ Misses 70820 43680 -27140
Partials 150 150
Flags with carried forward coverage won't be shown. Click here to find out more. |
Thank you for doing this! The description sound like the desired behavior, but I don't really know this part of the code base (or go or sql), so I will leave the technical review to someone else. |
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.
looks pretty good just some questions around if we can make any simplifications to this
master/static/migrations/20240321174101_handle-single-run-experiments-best-trial.tx.up.sql
Outdated
Show resolved
Hide resolved
master/static/migrations/20240321174101_handle-single-run-experiments-best-trial.tx.up.sql
Outdated
Show resolved
Hide resolved
master/static/migrations/20240321174101_handle-single-run-experiments-best-trial.tx.up.sql
Outdated
Show resolved
Hide resolved
512c8f5
to
cb9948e
Compare
master/static/migrations/20240617104613_handle-single-run-experiments-best-trial.tx.up.sql
Outdated
Show resolved
Hide resolved
7939f76
to
abd4a60
Compare
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.
LGTM
@corban-beaird what is the status of the PR? |
Right now my focus is on 2 high priority items for CM, once these are complete I'll be coming back to this PR and resolving the CI failures. I should be looking at having those items taken care by the end of this week at the latest; so currently on track to get this in prior to the release branch being cut this upcoming Tuesday. |
…ith no validation metrics
abd4a60
to
2f665eb
Compare
Description
Currently if a single-run/trial experiment doesn't store validation metrics, then it will not be assigned a best_trial_id, so when there are single trial experiments that are using
report_metrics
and do not use the validation metrics group, features like experiment comparison are unable to display data.This PR adds an additional trigger, and backfill logic, that runs on experiment completions that have no
best_trial_id
assigned. This will then populate thebest_trial_id
field if it's a single run/trial experiment.Test Plan
master/internal/api_experiment_intg_test.go
Commentary (optional)
Checklist
docs/release-notes/
.See Release Note for details.
Ticket
ET-115