feat: migrate image list #2615

gahyuun · 2024-08-07T10:04:36Z

This PR resolves #2586 issue

TL;DR

migrate ImageList

Why make this change?

To migrate from lit component to react component

What changed?

ImageInstallModal
- Component that install images
- I deselected the rows when clicking the install button in the modal because I felt the flow of deselecting the rows was more natural, whereas before it would not deselect the rows.
- The newly added phrases are written in en and ko only
- However, I didn't proceed with replacing "download" with "install". This was because there were more download phrases than I thought, and I thought it might be more confusing if I replaced a lot of them
ImageList
- I used suspense to prevent the entire screen from loading when the image list is first rendered
ManageAppsModal, ManageImageResourceLimitModal
- I modified it because it's called from a React component, not a lit component.
- Implemented the getServicePorts function in ManageAppsModal without passing service port as a prop
ResourceNumber
- I added ranges via the range and max properties ( ex 1~∞ Core )
- The ResourceNumber component made rendering the table too slow, so I used Memo

How to test?

imageList

enter the environment page
select the row
sort the column

ManageImageResourceLimitModal

click button
change resource
click Okay button
check if the resource has changed

ManageAppsModal

click button
add port
delete port
check if the ports has changed

ImageInstallModal

select uninstalled images
click install button
check uninstalled images info
click install button and check if an notification appears
select installed images
check warn message
select uninstalled images and installed images
check exclude message

graphite-app · 2024-08-07T10:04:42Z

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 · 2024-08-07T10:07:44Z

Coverage report for `./react`

St.^❔	Category	Percentage	Covered / Total
🔴	Statements	5.27% (+0.54% 🔼)	322/6109
🔴	Branches	4.96% (+0.22% 🔼)	208/4191
🔴	Functions	3.03% (+0.36% 🔼)	61/2013
🔴	Lines	5.16% (+0.55% 🔼)	308/5965

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / ImageInstallModal.tsx	0%	0%	0%	0%
🔴	`...` / ImageList.tsx	0%	0%	0%	0%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / ResourceNumber.tsx	26.67% (-4.1% 🔻)	0%	0%	26.67% (-4.1% 🔻)
🔴	`...` / FlexActivityIndicator.tsx	0%	0% (-100% 🔻)	0%	0%

Test suite run success

82 tests passing in 9 suites.

Report generated by 🧪jest coverage report action from 7156dd4

react/src/components/ImageList.tsx

react/src/index.tsx

react/src/pages/EnvironmentPage.tsx

react/src/components/ImageList.tsx

ironAiken2

Currently, we don't have pagination, so It's getting too much data and it's causing lag on page navigation and other interactions. Can you take a look at this as well?

gahyuun · 2024-08-09T07:59:12Z

react/src/components/ImageInstallModal.tsx

+    return null;
+  }
+
+  const handleClick = () => {


handleClick references the _installImage function in the lit code
https://github.com/lablup/backend.ai-webui/blob/main/src/components/backend-ai-environment-list.ts#L618

gahyuun · 2024-08-09T08:02:01Z

react/src/components/ImageList.tsx

+      key: 'installed',
+      sorter: sortBasedOnInstalled,
+      render: (text, row) => {
+        if (row?.id && installingImages.includes(row.id))


I used the installingImages state to show the installing tag

gahyuun · 2024-08-09T08:09:18Z

react/src/components/ImageList.tsx

+  const sortBasedOnInstalled = (a: EnvironmentImage, b: EnvironmentImage) => {
+    return a?.installed && !b?.installed
+      ? -1
+      : !a?.installed && b?.installed
+        ? 1
+        : 0;
+  };
+


https://github.com/lablup/backend.ai-webui/blob/main/src/components/backend-ai-environment-list.ts#L461
lit code for reference

gahyuun · 2024-08-09T08:19:32Z

resources/image_metadata.json

While checking ImageList, I added the parts that didn't have metaData.

gahyuun · 2024-08-09T08:28:25Z

react/src/hooks/index.tsx

@@ -251,7 +260,49 @@ export const useBackendAIImageMetaData = () => {
        const { tags } = getImageMeta(imageName);
        return tags[1];
      },
+      getBaseImages: (tag: string, name: string) => {


getBaseImages,getConstraints,getLangs implemented a new function without modifying the getBaseImage,getCustomTag,getFilteredRequirementsTag,getImageLang because of side effects ( ex. SessionKernelTags uses getBaseImage. )
Also, because of the side effect, I didn't modify MyEnvironmentPage to any values except the constraints. I think it's better to do this in a new task.

gahyuun · 2024-08-09T08:31:40Z

react/src/hooks/index.tsx

https://github.com/lablup/backend.ai-webui/blob/main/src/components/backend-ai-environment-list.ts#L345
lit code for reference

ironAiken2

Please resolve conflict :)

gahyuun · 2024-08-12T01:09:01Z

react/src/hooks/useBackendAIImageMetaData.test.tsx

+  (global as any).fetch = jest.fn(async () =>
+    Promise.resolve({
+      ok: true,
+      status: 200,
+      json: () =>
+        Promise.resolve({
+          imageInfo: {},
+          tagAlias: {
+            pytorch: 'PyTorch',
+            r: 'R',
+            'r-base': 'R',
+            py3: 'Python3',
+            ngc: 'Nvidia GPU Cloud',
+            py310: 'Python3',
+          },
+          tagReplace: {},
+        }),
+    }),
+  );


I used jest.fn because suspense can't wait for the test to complete while fetching data

react/src/pages/EnvironmentPage.tsx

react/src/components/ImageInstallModal.tsx

react/src/components/ImageList.tsx

react/src/components/ImageInstallModal.tsx

ironAiken2 · 2024-08-12T02:25:36Z

react/src/components/ImageInstallModal.tsx

+          globalThis.lablupNotification.text = painKiller.relieve(error.title);
+          // @ts-ignore
+          globalThis.lablupNotification.detail = error.message;
+          // @ts-ignore
+          globalThis.lablupNotification.show(true, error);
+          indicator.set(100, t('environment.DescProblemOccurred'));
+          indicator.end(1000);


Is there a reason why you didn't use upsertNotification in this part?

The way to display the error in upsertNotification seemed to be to set the backgroundTask status to rejected.
But there is no backgroundTask.

I also referenced the container Registry List code
ref) https://github.com/lablup/backend.ai-webui/blob/main/react/src/components/ContainerRegistryList.tsx#L159

@gahyuun we can change this part using upsertNotification with promise. This is an example.

ironAiken2 · 2024-08-12T02:33:49Z

react/src/components/ImageInstallModal.tsx

+    setInstallingImages(
+      imagesToInstall
+        .map((item) => item?.id)
+        .filter((id): id is string => id !== null && id !== undefined),
+    );


Isn't it duplicated with ref? And it seems that the problem will occur if you download another image additionally without refreshing the page.

I modified the rendering to only show the successful API image after clicking the install button.
I made the requests in parallel and only updated the state of the successful images.

ded9e49

react/src/components/ImageList.tsx

gahyuun · 2024-08-12T08:52:53Z

react/src/components/FlexActivityIndicator.tsx

I modified it because I wanted to resize the spinner

gahyuun · 2024-08-12T08:56:39Z

react/src/components/ImageList.tsx

+const CellWrapper = ({
+  children,
+  style,
+}: {
+  children: ReactNode;
+  style?: React.CSSProperties;
+}) => {
+  if (!children) return null;
+  return (
+    <div
+      style={{
+        display: 'flex',
+        alignItems: 'center',
+        height: '100%',
+        ...style,
+      }}
+    >
+      {children}
+    </div>
+  );
+};


In Ant Design's Table component, when using virtual mode and specifying a scroll.y value, the vertical center alignment does not seem to be applied correctly because the cells have a fixed height.
So, I implemented a component called CellWrapper and did the vertical alignment myself.

Since I was doing a custom render, the ellipsis property was not applied, so I applied ellipsis directly as well

gahyuun · 2024-08-12T09:03:01Z

react/src/components/ImageList.tsx

+          rowKey="id"
+          scroll={{
+            x: 1600,
+            y: window.innerHeight - 145 - 56 - 54,


145: ref ) https://github.com/lablup/backend.ai-webui/pull/2615/files#diff-7513e7364d1bbef624ab929ba47fa51f2fcb5c0090f36db4f56e3837bb29bac5R78

56 : The space where the install button exists

54: column title of table

rapsealk · 2024-08-12T15:33:17Z

Please resolve the conflicts!

yomybaby

I'm working to resolve blow list:

Make the entire row clickable.
Resource Limitation Column: The rows in this table do not have a fixed height, so 'virtual' is not appropriate. Changing to a fixed height is not possible due to the resource limitation UI column.
Base and Constraint Columns: Display them in one line.
When you modify an image, the order can be changed. To prevent this, let's set the order using 'humanized_name'.

I'll share soon.

yomybaby

I got an error when I try to update using Manage app modal. Please check it again.

yomybaby

LGTM

ironAiken2 · 2024-08-27T04:15:40Z

react/src/components/ImageList.tsx

+              }
+            },
+          })}
+          showSorterTooltip={false}


Suggested change

showSorterTooltip={false}

showSorterTooltip={false}

sortDirections={['descend', 'ascend', 'descend']}

Logic to limit data sorting to ascending and descending order is not needed in the webui?

ironAiken2

LGTM

graphite-app · 2024-08-27T09:57:46Z

Merge activity

Aug 27, 5:57 AM EDT: yomybaby added this pull request to the Graphite merge queue.
Aug 27, 6:01 AM EDT: yomybaby merged this pull request with the Graphite merge queue.

### This PR resolves [#2586 ](#2586) issue ### TL;DR migrate ImageList ### Why make this change? To migrate from lit component to react component ### What changed? - ImageInstallModal - Component that install images - I deselected the rows when clicking the install button in the modal because I felt the flow of deselecting the rows was more natural, whereas before it would not deselect the rows. - The newly added phrases are written in en and ko only - However, I didn't proceed with replacing "download" with "install". This was because there were more download phrases than I thought, and I thought it might be more confusing if I replaced a lot of them - ImageList - I used suspense to prevent the entire screen from loading when the image list is first rendered - ManageAppsModal, ManageImageResourceLimitModal - I modified it because it's called from a React component, not a lit component. - Implemented the getServicePorts function in ManageAppsModal without passing service port as a prop - ResourceNumber - I added ranges via the range and max properties ( ex 1~∞ Core ) - The ResourceNumber component made rendering the table too slow, so I used Memo ### How to test? - imageList 1. enter the environment page 2. select the row 3. sort the column - ManageImageResourceLimitModal 1. click button 3. change resource 4. click Okay button 5. check if the resource has changed - ManageAppsModal 1. click button 2. add port 3. delete port 4. check if the ports has changed - ImageInstallModal 1. select uninstalled images 2. click install button 3. check uninstalled images info 4. click install button and check if an notification appears 1. select installed images 2. check warn message 1. select uninstalled images and installed images 2. check exclude message

### TL;DR After #2615, the Resource Number displays an infinity symbol unexpectedly. ![image.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/XqC2uNFuj0wg8I60sMUh/ee78810c-da7e-4312-be4a-09aa269931b9.png) This PR fixes that bug and refactors the code related to the `max` display.

gahyuun self-assigned this Aug 7, 2024

github-actions bot added area:ux UI / UX issue. area:i18n Localization size:XL 500~ LoC labels Aug 7, 2024

gahyuun requested review from ironAiken2 and yomybaby August 8, 2024 04:23

gahyuun marked this pull request as ready for review August 8, 2024 04:23

gahyuun marked this pull request as draft August 8, 2024 04:30

gahyuun marked this pull request as ready for review August 8, 2024 04:42

ironAiken2 reviewed Aug 8, 2024

View reviewed changes

react/src/components/ImageList.tsx Outdated Show resolved Hide resolved

ironAiken2 reviewed Aug 8, 2024

View reviewed changes

react/src/index.tsx Outdated Show resolved Hide resolved

ironAiken2 reviewed Aug 8, 2024

View reviewed changes

react/src/pages/EnvironmentPage.tsx Outdated Show resolved Hide resolved

ironAiken2 reviewed Aug 8, 2024

View reviewed changes

react/src/components/ImageList.tsx Outdated Show resolved Hide resolved

ironAiken2 reviewed Aug 8, 2024

View reviewed changes

react/src/components/ImageList.tsx Outdated Show resolved Hide resolved

ironAiken2 reviewed Aug 8, 2024

View reviewed changes

react/src/components/ImageList.tsx Outdated Show resolved Hide resolved

gahyuun marked this pull request as draft August 8, 2024 09:44

ironAiken2 requested changes Aug 8, 2024

View reviewed changes

gahyuun marked this pull request as ready for review August 9, 2024 04:54

gahyuun requested a review from ironAiken2 August 9, 2024 05:16

gahyuun force-pushed the refactor/migrate-image-list branch from 77c19ae to 77064b1 Compare August 9, 2024 05:37

gahyuun commented Aug 9, 2024

View reviewed changes

ironAiken2 requested changes Aug 9, 2024

View reviewed changes

gahyuun commented Aug 12, 2024

View reviewed changes

gahyuun requested a review from ironAiken2 August 12, 2024 01:09