-
Notifications
You must be signed in to change notification settings - Fork 356
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
feat: Add Resource Allocation stats to trial detail #9533
Conversation
✅ Deploy Preview for determined-ui canceled.
|
im not a reviewer but i remembered we've decided not to show the allocation images as of this March because that causes design collapse with many slots. Is that fixed? |
@keita-determined Thanks for the information. As far as I knew, Aleph Alpha is still requesting this feature, also we decided to only show the used slots (blue part), and ignore the total/available slots (grey part) |
@gt2345 got it, thanks |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9533 +/- ##
==========================================
+ Coverage 51.31% 51.35% +0.04%
==========================================
Files 1252 1252
Lines 152028 152173 +145
Branches 3019 3023 +4
==========================================
+ Hits 78007 78150 +143
- Misses 73862 73865 +3
+ Partials 159 158 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
}, [canceler, stopPolling]); | ||
|
||
useEffect(() => { | ||
if (acceleratorData && !pausableRunStates.has(experiment.state)) stopPolling(); |
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.
nice
@@ -341,7 +345,14 @@ func (a *apiServer) GetTaskAcceleratorData( | |||
|
|||
var accelerationData []*apiv1.AcceleratorData | |||
for _, r := range res { | |||
accelerationData = append(accelerationData, r.Proto()) | |||
accelerationData = append(accelerationData, &apiv1.AcceleratorData{ |
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.
why not modify the Proto() method?
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'm hesitate to change the model.AcceleratorData
type, it will no longer match the allocation_accelerators
table, also there are use cases where the resource_pool
field is not necessary
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
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
@@ -595,6 +595,8 @@ message AcceleratorData { | |||
string accelerator_type = 5; | |||
// An array of UUIDs of the accelerators associated with the allocation. | |||
repeated string accelerator_uuids = 6; | |||
// The name of the resource pool. | |||
string resource_pool = 7; |
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.
its a little awkward to me that we won't use this field when calling v1PostAllocationAcceleratorDataRequest
but I think this is fine
ResourcePool string | ||
}{} | ||
|
||
err := db.Bun().NewSelect().ColumnExpr("alloc_acc.*"). |
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.
can you update a test in api_tasks_intg_test.go
to check resource pool is set right?
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.
Of course, test added
Ticket
MD-440
Description
Add
Resource Allocation
information to trial details page. When clicking on the stats card, a modal will display allocation details. The font of the card is the theme color, to indicate it's clickable.For a local cpu experiment
More screenshots using mock data
Test Plan
Checklist
docs/release-notes/
See Release Note for details.