fix: report errors from deletecheckpoints endpoint + improve feedback #9178
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9178 +/- ##
==========================================
- Coverage 45.71% 37.10% -8.61%
==========================================
Files 1179 600 -579
Lines 146795 93617 -53178
Branches 2419 2420 +1
==========================================
- Hits 67110 34741 -32369
+ Misses 79471 58662 -20809
Partials 214 214
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -128,12 +126,13 @@ const ExperimentCheckpoints: React.FC<Props> = ({ experiment, pageRef }: Props) | |||
| [registerModal], | |||
| ); | |||
|
|
|||
| const handleDelete = useCallback((checkpoints: string[]) => { | |||
| readStream( | |||
There was a problem hiding this comment.
qq: do you know why it used to use readStream
There was a problem hiding this comment.
the copy implies that we purposefully dropped the error messaging -- i think that's why we used readstream, because its default behavior is to drop errors
keita-determined
added
the
to-cherry-pick
master/internal/api_checkpoint.go
Outdated
| } | ||
| checkpointMsgs := make([]string, len(registeredCheckpointUUIDs)) | ||
| i = 0 | ||
| for k, v := range registeredCheckpointUUIDs { | ||
| if _, ok := accessibleModels[v.ID]; ok { | ||
| checkpointMsgs[i] = fmt.Sprintf("%v, registered to %v (model #%d), version %d", k, v.Name, v.ID, v.Version) | ||
| } else { | ||
| checkpointMsgs[i] = fmt.Sprintf("%v, registered to an unknown model", k) | ||
| } | ||
| i++ | ||
| } |
There was a problem hiding this comment.
this is very strange go style, a micro-optimization not worth the cost of readability. can we write this instead:
| } | |
| checkpointMsgs := make([]string, len(registeredCheckpointUUIDs)) | |
| i = 0 | |
| for k, v := range registeredCheckpointUUIDs { | |
| if _, ok := accessibleModels[v.ID]; ok { | |
| checkpointMsgs[i] = fmt.Sprintf("%v, registered to %v (model #%d), version %d", k, v.Name, v.ID, v.Version) | |
| } else { | |
| checkpointMsgs[i] = fmt.Sprintf("%v, registered to an unknown model", k) | |
| } | |
| i++ | |
| } | |
| } | |
| var checkpointMsgs []string | |
| for k, v := range registeredCheckpointUUIDs { | |
| if _, ok := accessibleModels[v.ID]; ok { | |
| msg := fmt.Sprintf("%v, registered to %v (model #%d), version %d", k, v.Name, v.ID, v.Version) | |
| checkpointMsgs = append(checkpointMsgs, msg) | |
| } else { | |
| msg := fmt.Sprintf("%v, registered to an unknown model", k) | |
| checkpointMsgs = append(checkpointMsgs, msg) | |
| } | |
| } |
There was a problem hiding this comment.
var checkpointMsgs []string part can be checkpointMsgs := make([]string, 0, len(registeredCheckpointUUIDs))? i think thats what Ashton meant. then we can avoid using index access and also no unnecessary memory allocation i guess
There was a problem hiding this comment.
yeah, that works too. but still is a micro-optimization. preallocating it with cap len(registeredCheckpointUUIDs) vs just var x []T doesn't matter unless you've benchmarked that it does. i'm sure it is nothing compared to the sql query above it.
master/internal/api_checkpoint.go
Outdated
| modelIDs := make([]int, len(registeredCheckpointUUIDs)) | ||
| i := 0 | ||
| for _, v := range registeredCheckpointUUIDs { | ||
| modelIDs[i] = v.ID | ||
| i++ | ||
| } |
|
checked and both current failures are due to ci issues being tracked in slack |
Ticket
ET-169
Description
This fixes an issue where errors from the delete checkpoints endpoint were not reported in the ui. We fix that and also improve the error messaging such that the user knows which model version the checkpoint is registered to.
Test Plan
Checklist
docs/release-notes/.See Release Note for details.