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

fix/consider-version-compatibility-on-extra-mounts-in-model-serving #2447

Conversation

lizable
Copy link
Contributor

@lizable lizable commented May 30, 2024

This PR provides conditionally showing extra mount vfolders only when Core version supports its feature.

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
Contributor Author

lizable commented May 30, 2024

@lizable lizable marked this pull request as ready for review May 30, 2024 08:07
@lizable lizable requested review from ironAiken2 and yomybaby May 30, 2024 08:08
@lizable lizable force-pushed the 05-30-feature_add-extra-mount-and-mount-destination-on-routing-info branch from 3ddb6c1 to 8dcb937 Compare May 30, 2024 09:32
@lizable lizable force-pushed the 05-30-fix_consider-version-compatibility-on-extra-mounts-in-model-serving branch from fdb5d30 to b49a335 Compare May 30, 2024 09:33
@lizable lizable force-pushed the 05-30-fix_consider-version-compatibility-on-extra-mounts-in-model-serving branch from 67ee4af to 2b1381f Compare May 30, 2024 09:53
@lizable lizable requested a review from yomybaby May 30, 2024 09:55
Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

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

Please hide some form items based on compatibility.
When I tested with 24.03.0b1 manager, I can see new fields.
image.png

@lizable lizable requested a review from yomybaby May 30, 2024 15:47
@lizable lizable requested a review from yomybaby May 31, 2024 02:28
Copy link
Contributor Author

lizable commented May 31, 2024

I fixed the minor issue which is spotted by @yomybaby,
related to accessibility of vfolder. By enabling edit feature only if the current logged in user and creator of model service is identical fixes it.

Screenshot 2024-05-31 at 12.51.22 PM.png

Screenshot 2024-05-31 at 12.51.33 PM.png

Comment on lines 340 to 346
{_.map(endpoint?.extra_mounts, (vfolder: VFolder) => {
return (
<Flex direction="row" gap={'xxs'}>
<FolderOutlined /> {vfolder.name}
</Flex>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{_.map(endpoint?.extra_mounts, (vfolder: VFolder) => {
return (
<Flex direction="row" gap={'xxs'}>
<FolderOutlined /> {vfolder.name}
</Flex>
{_.map(endpoint?.extra_mounts, (vfolder) => {
return (
<Flex direction="row" gap={'xxs'}>
<FolderOutlined /> {vfolder?.name}
</Flex>

Copy link
Contributor

@ironAiken2 ironAiken2 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

yomybaby commented May 31, 2024

Merge activity

  • May 31, 2:51 AM EDT: @yomybaby started a stack merge that includes this pull request via Graphite.
  • May 31, 2:56 AM EDT: Graphite rebased this pull request as part of a merge.
  • May 31, 2:58 AM EDT: @yomybaby merged this pull request with Graphite.

@yomybaby yomybaby force-pushed the 05-30-feature_add-extra-mount-and-mount-destination-on-routing-info branch from 75de60a to a70f290 Compare May 31, 2024 06:53
Base automatically changed from 05-30-feature_add-extra-mount-and-mount-destination-on-routing-info to main May 31, 2024 06:55
@yomybaby yomybaby force-pushed the 05-30-fix_consider-version-compatibility-on-extra-mounts-in-model-serving branch from 4de9bc2 to a1316d0 Compare May 31, 2024 06:56
Copy link

Coverage report for ./react

St.
Category Percentage Covered / Total
🔴 Statements
2.85% (-0% 🔻)
140/4915
🔴 Branches
3.31% (-0.02% 🔻)
110/3324
🔴 Functions
1.55% (-0% 🔻)
25/1613
🔴 Lines
2.74% (-0% 🔻)
132/4825

Test suite run success

48 tests passing in 5 suites.

Report generated by 🧪jest coverage report action from a1316d0

@yomybaby yomybaby merged commit bfb86e5 into main May 31, 2024
7 checks passed
@yomybaby yomybaby deleted the 05-30-fix_consider-version-compatibility-on-extra-mounts-in-model-serving branch May 31, 2024 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:lib Library and SDK related issue. size:L 100~500 LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants