Skip to content
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: Session usage monitor in react #2814

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ironAiken2
Copy link
Contributor

@ironAiken2 ironAiken2 commented Nov 5, 2024

This PR resolves #2803 issue.

Changes:

  • Displays the resources allocated to the session. Currently, the compute session node does not include a live stat field, which shows the live stat information of the main kernel associated with the session.
  • Adjusts the number of Cols to be displayed in one Row based on the width of the drawer. On screen sizes smaller than md, 1Col is displayed in 1Row.
  • Shows the appropriate icon for each accelerator device. For util, it shows just the percentage, and for memory on the device, it shows the specific usage.

How to test:

  • Access the Detail page of the endpoint that is currently in service.
  • Verify that resource usage looks normal.
image.png image.png

Checklist: (if applicable)

  • Mention to the original issue
  • Documentation
  • Minium required manager version
  • Specific setting for review (eg., KB link, endpoint or how to setup)
  • Minimum requirements to check during review
  • Test case(s) to demonstrate the difference of before/after

Copy link

graphite-app bot commented Nov 5, 2024

Your org requires the Graphite merge queue for merging into main

Add the label “flow:merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “flow:hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the size:L 100~500 LoC label Nov 5, 2024
Copy link
Contributor Author

ironAiken2 commented Nov 5, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

github-actions bot commented Nov 5, 2024

Coverage report for ./react

St.
Category Percentage Covered / Total
🔴 Statements
5.26% (-0.02% 🔻)
395/7503
🔴 Branches
4.54% (-0.04% 🔻)
237/5220
🔴 Functions
3.15% (-0.01% 🔻)
78/2475
🔴 Lines
5.18% (-0.02% 🔻)
380/7331
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🔴
... / SessionUsageMonitor.tsx
0% 0% 0% 0%

Test suite run success

124 tests passing in 14 suites.

Report generated by 🧪jest coverage report action from a1bc1c6

@ironAiken2 ironAiken2 marked this pull request as ready for review November 5, 2024 09:13
Copy link
Contributor

@agatha197 agatha197 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image.png
Memory doesn't have label, but CUDA memory has label.

react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
react/src/components/SessionUsageMonitor.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@agatha197 agatha197 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

</Row>
<Flex justify="end" style={{ width: '100%' }}>
<Typography.Text>
{`I/O Read: ${iSizeToSize(sortedLiveStat?.io_read?.current, 'g')?.numberUnit}B / Write: ${iSizeToSize(sortedLiveStat?.io_write?.current, 'g')?.numberUnit}B`}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The iSizeToSize function is for binary size conversion, not for I/O. Please check the original version and ensure that the display matches it. See the reference here: https://github.com/lablup/backend.ai-webui/blob/main/src/components/backend-ai-session-list.ts#L4318-L4326

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that code, the API request value comes in as a MB value, but not in ComputeSessionNode, so I modified it to show it in MB, the same as the session list usage column, using convertDecimalSizeUnit.

Comment on lines 47 to 62
const resourceTypeMap: {
[key in acceleratorType | 'cpu' | 'mem']: string;
} = {
cpu: 'cpu',
mem: 'mem',
cuda: 'cuda.device',
rocm: 'rocm.device',
tpu: 'tpu.device',
ipu: 'ipu.device',
atom: 'atom.device',
'atom-plus': 'atom-plus.device',
gaudi2: 'gaudi2.device',
warboy: 'warboy.device',
rngd: 'rngd.device',
'hyperaccel-lpu': 'hyperaccel-lpu.device',
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to create a function to add device without using hardcoded accelerator names.

Comment on lines 202 to 208
{_.map(sortedLiveStat, (value, key) => {
const device = key.split('_')?.[0] as keyof typeof resourceTypeMap;

return _.includes(
_.keysIn(_.omit(resourceTypeMap, 'cpu', 'mem')),
device,
) ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's refactor this part. filter(or pickBy) first and map later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L 100~500 LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Session usage component in React
3 participants