fix: API migration to improve performance in resource pool page #9056

keita-determined · 2024-03-26T20:40:49Z

Description

ET-104
Related to #9048

Test Plan

Check if Slots Allocated bars show the correct data
Check if topology works
- Check if topology doesn't show up if nodes > 1,000 OR slots > 10,000
- Check if polling is working when topology is available
Check if test cases covers edge cases

Commentary (optional)

Merge this after #9048

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

netlify · 2024-03-26T20:41:08Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`0d6943a`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6604f022320c4d0008990f24
😎 Deploy Preview	https://deploy-preview-9056--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

hamidzr

lgtm we want to make sure agent.slots is not used anywhere in the cluster page but the topology view.

webui/react/src/stores/cluster.tsx

webui/react/src/types.ts

webui/react/src/stores/cluster.tsx

hamidzr · 2024-03-27T01:47:13Z

webui/react/src/services/apiConfig.ts

@@ -481,7 +481,8 @@ export const getTelemetry: DetApi<EmptyParams, Api.V1GetTelemetryResponse, Telem
 export const getAgents: DetApi<EmptyParams, Api.V1GetAgentsResponse, Type.Agent[]> = {
  name: 'getAgents',
  postProcess: (response) => decoder.jsonToAgents(response.agents || []),
-  request: () => detApi.Cluster.getAgents(),
+  request: () =>
+    detApi.Cluster.getAgents(undefined, undefined, undefined, undefined, undefined, true),


I have some changes in this branch where I was working toward parameterizing getting slots so that we can still use it for topology in case it helps

that branch also includes some changes for disabling topology

basically if and only if agents < 1000 && slotCount < 10000 then we want to pull down slots and render toplogy

codecov · 2024-03-27T02:16:35Z

Codecov Report

Attention: Patch coverage is 22.96296% with 104 lines in your changes are missing coverage. Please review.

Project coverage is 40.57%. Comparing base (18dd29e) to head (0d6943a).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9056      +/-   ##
==========================================
- Coverage   47.16%   40.57%   -6.59%     
==========================================
  Files        1150      829     -321     
  Lines      141674   102591   -39083     
  Branches     2415     2416       +1     
==========================================
- Hits        66814    41630   -25184     
+ Misses      74670    60771   -13899     
  Partials      190      190

Flag	Coverage Δ
harness	`?`
web	`38.92% <22.96%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
webui/react/src/utils/cluster.ts	`100.00% <100.00%> (ø)`
webui/react/src/services/decoder.ts	`19.95% <0.00%> (-0.03%)`	⬇️
webui/react/src/types.ts	`99.66% <66.66%> (-0.34%)`	⬇️
webui/react/src/services/types.ts	`0.00% <0.00%> (ø)`
webui/react/src/services/api.ts	`0.00% <0.00%> (ø)`
webui/react/src/services/apiConfig.ts	`72.56% <27.27%> (-0.29%)`	⬇️
webui/react/src/stores/cluster.tsx	`37.74% <34.78%> (+0.95%)`	⬆️
...eact/src/pages/ResourcePool/ResourcepoolDetail.tsx	`0.00% <0.00%> (ø)`

... and 321 files with indirect coverage changes

hamidzr · 2024-03-27T02:33:19Z

something looks wrong with the topology:

before change

after change

webui/react/src/pages/ResourcePool/Topology.tsx

webui/react/src/stores/cluster.tsx

hamidzr · 2024-03-27T02:44:31Z

thanks for working on this! I posted some questions and comments

webui/react/src/pages/ResourcePool/ResourcepoolDetail.tsx

webui/react/src/utils/cluster.ts

webui/react/src/pages/ResourcePool/ResourcepoolDetail.tsx

ashtonG

javascript looks good

hamidzr · 2024-03-27T20:41:00Z

I can see the toplogy turning on and off as expected. but I also do see a request to fetch all the slots in a scenario I wasn't expecting

this case has 1m slots

determined/webui/react/src/pages/ResourcePool/ResourcepoolDetail.tsx

Line 68 in 921bd0d

const agents = Loadable.getOrElse([], useObservable(clusterStore.agents));

probably needs to check that agents are loaded before going past this check?

determined/webui/react/src/pages/ResourcePool/ResourcepoolDetail.tsx

Lines 96 to 98 in 921bd0d

    
           if (agents.length > MAX_USABLE_NODES || totalSlots > MAX_USABLE_SLOTS) { 
        
             return []; 
        
           }

webui/react/src/pages/ResourcePool/ResourcepoolDetail.tsx

hamidzr

thanks for the update. I can see it pulling down the slots fine now when it should. But also we'd want it to keep refreshing those and continue to keep requesting for them. I assume that's needed for the topology to show the latest data

hamidzr

thanks for fixing that issue. I see the ui is double requesting agents now on a recurring basis we can say we want the latest numbers on agents to be able to turn topology on or off if the situation changes but maybe just relying on the initial agent response once is enough?
this only happens when the conditions to show the topology is met eg the cluster is smaller so it's probably fine?

after

before in main

keita-determined · 2024-03-28T16:51:09Z

thanks for fixing that issue. I see the ui is double requesting agents now on a recurring basis we can say we want the latest numbers on agents to be able to turn topology on or off if the situation changes but maybe just relying on the initial agent response once is enough?
this only happens when the conditions to show the topology is met eg the cluster is smaller so it's probably fine?

i think this should be fine because both APIs are used for different purposes and calling APIs is cheaper than the original issue.

* squash squash * fix: API migration for resource pool * fix: removed unused code * fix: migrate `slotContainerStates` with the new agent schema * test: test cases for `getSlotContainerStates` * fix: better type * test: add edae case * fix: minor rebase fixes * feat: topology workaround * fix: minor fixes * fix: add condition to show topology * fix: feedback * fix: avoid call agent api if there are too many nodes or slots * fix: polling agent with slots --------- Co-authored-by: Hamid Zare <12127420+hamidzr@users.noreply.github.com> (cherry picked from commit 17287f5)

hamidzr · 2024-04-03T17:20:33Z

test steps for posterity
https://github.com/determined-ai/release-party-issues/issues/1658#issuecomment-2035167365

cla-bot bot added the cla-signed label Mar 26, 2024

keita-determined changed the base branch from main to hz-customerk March 26, 2024 20:41

keita-determined force-pushed the fix/web-resource-pools branch from 5021c86 to 45a7196 Compare March 26, 2024 21:26

hamidzr approved these changes Mar 26, 2024

View reviewed changes

webui/react/src/stores/cluster.tsx Outdated Show resolved Hide resolved

webui/react/src/types.ts Outdated Show resolved Hide resolved

keita-determined mentioned this pull request Mar 26, 2024

feat: add slot stats to /agents endpoints #9048

Merged

4 tasks

hamidzr force-pushed the hz-customerk branch from b7975fd to 6324a6c Compare March 26, 2024 23:55

keita-determined force-pushed the fix/web-resource-pools branch from 2b9ca08 to fdf1093 Compare March 27, 2024 00:27

keita-determined commented Mar 27, 2024

View reviewed changes

webui/react/src/stores/cluster.tsx Show resolved Hide resolved

keita-determined force-pushed the fix/web-resource-pools branch from 3102b90 to ae73ef6 Compare March 27, 2024 01:07

Base automatically changed from hz-customerk to main March 27, 2024 01:16

keita-determined force-pushed the fix/web-resource-pools branch from ae73ef6 to 7d1baf9 Compare March 27, 2024 01:24

hamidzr reviewed Mar 27, 2024

View reviewed changes

hamidzr added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Mar 27, 2024

keita-determined force-pushed the fix/web-resource-pools branch 2 times, most recently from 29719a0 to 19fe58e Compare March 27, 2024 02:13

keita-determined marked this pull request as ready for review March 27, 2024 02:13

keita-determined requested a review from a team as a code owner March 27, 2024 02:13

keita-determined requested a review from EmilyBonar March 27, 2024 02:13

keita-determined force-pushed the fix/web-resource-pools branch from 19fe58e to 2f5b3c6 Compare March 27, 2024 02:17

hamidzr reviewed Mar 27, 2024

View reviewed changes

webui/react/src/pages/ResourcePool/Topology.tsx Outdated Show resolved Hide resolved

hamidzr reviewed Mar 27, 2024

View reviewed changes

webui/react/src/pages/ResourcePool/Topology.tsx Outdated Show resolved Hide resolved

hamidzr reviewed Mar 27, 2024

View reviewed changes

webui/react/src/stores/cluster.tsx Outdated Show resolved Hide resolved

mackrorysd self-assigned this Mar 27, 2024

keita-determined requested a review from hamidzr March 27, 2024 19:02

keita-determined force-pushed the fix/web-resource-pools branch 2 times, most recently from 4760395 to 80f82ba Compare March 27, 2024 19:22

keita-determined added 7 commits March 27, 2024 12:35

test: test cases for getSlotContainerStates

caa4c62

fix: better type

15a5973

test: add edae case

6ff3c75

fix: minor rebase fixes

d5dda67

feat: topology workaround

40b4b37

fix: minor fixes

482fd89

fix: add condition to show topology

ca558d3

keita-determined force-pushed the fix/web-resource-pools branch from 80f82ba to ca558d3 Compare March 27, 2024 19:35

ashtonG reviewed Mar 27, 2024

View reviewed changes

hamidzr reviewed Mar 27, 2024

View reviewed changes

webui/react/src/pages/ResourcePool/ResourcepoolDetail.tsx Outdated Show resolved Hide resolved

fix: feedback

921bd0d

keita-determined requested review from hamidzr and ashtonG March 27, 2024 20:18

ashtonG reviewed Mar 27, 2024

View reviewed changes

hamidzr reviewed Mar 27, 2024

View reviewed changes

webui/react/src/pages/ResourcePool/ResourcepoolDetail.tsx Show resolved Hide resolved

hamidzr reviewed Mar 27, 2024

View reviewed changes

webui/react/src/pages/ResourcePool/ResourcepoolDetail.tsx Outdated Show resolved Hide resolved

ashtonG approved these changes Mar 27, 2024

View reviewed changes

keita-determined requested a review from hamidzr March 27, 2024 23:53

fix: avoid call agent api if there are too many nodes or slots

ce5f792

keita-determined force-pushed the fix/web-resource-pools branch from 7cc080a to ce5f792 Compare March 28, 2024 00:00

hamidzr requested changes Mar 28, 2024

View reviewed changes

fix: polling agent with slots

0d6943a

keita-determined requested a review from hamidzr March 28, 2024 04:20

hamidzr approved these changes Mar 28, 2024

View reviewed changes

keita-determined merged commit 17287f5 into main Mar 28, 2024
71 of 83 checks passed

keita-determined deleted the fix/web-resource-pools branch March 28, 2024 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: API migration to improve performance in resource pool page #9056

fix: API migration to improve performance in resource pool page #9056

keita-determined commented Mar 26, 2024 •

edited by jira bot

Loading

netlify bot commented Mar 26, 2024 •

edited

Loading

hamidzr left a comment

hamidzr Mar 27, 2024 •

edited

Loading

codecov bot commented Mar 27, 2024 •

edited

Loading

hamidzr commented Mar 27, 2024 •

edited

Loading

hamidzr commented Mar 27, 2024

ashtonG left a comment

hamidzr commented Mar 27, 2024 •

edited

Loading

hamidzr left a comment

hamidzr left a comment

keita-determined commented Mar 28, 2024

hamidzr commented Apr 3, 2024

fix: API migration to improve performance in resource pool page #9056

fix: API migration to improve performance in resource pool page #9056

Conversation

keita-determined commented Mar 26, 2024 • edited by jira bot Loading

Description

Test Plan

Commentary (optional)

Checklist

Ticket

netlify bot commented Mar 26, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

hamidzr left a comment

Choose a reason for hiding this comment

hamidzr Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 27, 2024 • edited Loading

Codecov Report

hamidzr commented Mar 27, 2024 • edited Loading

hamidzr commented Mar 27, 2024

ashtonG left a comment

Choose a reason for hiding this comment

hamidzr commented Mar 27, 2024 • edited Loading

hamidzr left a comment

Choose a reason for hiding this comment

hamidzr left a comment

Choose a reason for hiding this comment

keita-determined commented Mar 28, 2024

hamidzr commented Apr 3, 2024

keita-determined commented Mar 26, 2024 •

edited by jira bot

Loading

netlify bot commented Mar 26, 2024 •

edited

Loading

hamidzr Mar 27, 2024 •

edited

Loading

codecov bot commented Mar 27, 2024 •

edited

Loading

hamidzr commented Mar 27, 2024 •

edited

Loading

hamidzr commented Mar 27, 2024 •

edited

Loading