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

add: providing additional vfolder mounts and arbitrary model definition path in model service launcher #2437

Merged

Conversation

lizable
Copy link
Contributor

@lizable lizable commented May 28, 2024

  • Related: backend.ai#2218
  • Prerequisites:
    • all-set configuration to creating / modifying model-service.
    • wsproxy v2 configuration
    • model-definition.yaml file

This PR adds two features in model service launcher, supporting arbitrary path of model definition file (previously set to /models and model-definition.yaml(or yml) file, and providing additional mounts and aliasing on other vfolders.

⚠️ NOTE: When updating the model service, it doesn't applied immediately. It applies when there's no corresponding session
in running status, which represents "PROVISIONING" or "READY" status of model service.

Test

  • Check whether you can create model service with no additional configuration, just model folder with model-definition.yml file inside. (default model folder mount path would be /models)
  • Check whether you can create model service with setting arbitrary path of mount destination for model folder (e.g. /test)
  • Check whether you can create model service with setting user-defined model definition file in model folder to mount by renaming it.
  • Check whether you can mount a general type folder with aliasing if needed

Screenshot(s)

  • Creating model service

    After Before
    Screenshot 2024-05-29 at 10 22 41 PM Screenshot 2024-05-29 at 10 14 15 PM
  • Modifying model service

    After Before
    Screenshot 2024-05-29 at 10 18 46 PM Screenshot 2024-05-29 at 10 21 50 PM

Checklist: (if applicable)

  • Mention to the original issue
  • Documentation
  • Minimum 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

@github-actions github-actions bot added area:ux UI / UX issue. area:i18n Localization labels May 28, 2024
@github-actions github-actions bot added the size:L 100~500 LoC label May 28, 2024
Copy link
Contributor Author

lizable commented May 28, 2024

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

Join @lizable and the rest of your teammates on Graphite Graphite

@yomybaby yomybaby force-pushed the feature/refactor-model-storage-mount-in-model-service-launcher branch from 54d4865 to 34dea2a Compare May 28, 2024 03:13
@lizable lizable force-pushed the feature/refactor-model-storage-mount-in-model-service-launcher branch from 90e9d76 to d639592 Compare May 28, 2024 10:47
@lizable lizable added this to the 24.03 milestone May 29, 2024
@lizable lizable force-pushed the feature/refactor-model-storage-mount-in-model-service-launcher branch 2 times, most recently from 592b941 to 30e94b3 Compare May 29, 2024 07:56
@github-actions github-actions bot added size:XL 500~ LoC and removed size:L 100~500 LoC labels May 29, 2024
@lizable lizable changed the title add: draft for providing additional vfolder mounts in model service launcher add: providing additional vfolder mounts in model service launcher May 29, 2024
@lizable lizable changed the title add: providing additional vfolder mounts in model service launcher add: providing additional vfolder mounts and arbitrary model definition path in model service launcher May 29, 2024
@lizable lizable added type:enhance Add new features urgency:blocker IT SHOULD BE RESOLVED BEFORE DISTRIBUTING urgency:5 It is imperative that action be taken right away. type:refactor Refactoring current implementation. impact:breaking Breaking or highlighted changes. labels May 29, 2024
@lizable lizable requested review from agatha197, yomybaby and ironAiken2 and removed request for agatha197 May 29, 2024 13:20
Copy link

github-actions bot commented May 29, 2024

Coverage report for ./react

St.
Category Percentage Covered / Total
🔴 Statements
2.85% (-0.01% 🔻)
140/4905
🔴 Branches
3.33% (-0.01% 🔻)
110/3301
🔴 Functions
1.55% (-0.01% 🔻)
25/1611
🔴 Lines
2.74% (-0.01% 🔻)
132/4815

Test suite run success

48 tests passing in 5 suites.

Report generated by 🧪jest coverage report action from 1d25210

@lizable lizable marked this pull request as ready for review May 29, 2024 13:43
Comment on lines 822 to 795
}}
/>
</>
Copy link
Member

@yomybaby yomybaby May 29, 2024

Choose a reason for hiding this comment

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

You don't need an extra state to update some component based on the form's value. Instead of using an extra state, you can wrap the part you want to update using <Form.Item noStyle shouldUpdate ...>. After that, you can delete the selectedModelFolder state, updateSelectedModelFolder, and the useEffect for that.

Some like this(I have not tested below code.):

Suggested change
}}
/>
</>
<Form.Item noStyle shouldUpdate={(prev,cur)=>prev.vFolderName !== cur.vFolderName}>
{()=>{
return <VFolderTableFormItem
rowKey={'id'}
label={t('modelService.AdditionalMounts')}
filter={(vf) =>
vf.name !== formRef.current?.getFieldValue('vFolderName') &&
vf.status === 'ready' &&
vf.usage_mode !== 'model'
}
tableProps={{
size: 'small',
}}
/>
}}
</Form.Item>

@lizable lizable requested a review from yomybaby May 30, 2024 02:47
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 check below:

  • When the user enters Mount Destination For Model Folder and then makes it empty again, the user gets an error when submitting:
{
    "type": "https://api.backend.ai/probs/invalid-api-params",
    "title": "Missing or invalid API parameters.",
    "msg": "Alias name cannot be empty."
}

Copy link
Contributor Author

lizable commented May 30, 2024

For now, there's no way to import alias of extra mounted vfolders, unless fetching from server.

@lizable lizable force-pushed the feature/refactor-model-storage-mount-in-model-service-launcher branch from b8bcccc to 94cddaa Compare May 30, 2024 05:57
@lizable lizable requested a review from yomybaby May 30, 2024 08:08
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.

This feedback is also not resolved.

Comment on lines 783 to 794
<VFolderTableFormItem
rowKey={'id'}
label={t('modelService.AdditionalMounts')}
filter={(vf) =>
vf.name !== formRef.current?.getFieldValue('vFolderName') &&
vf.status === 'ready' &&
vf.usage_mode !== 'model'
}
tableProps={{
size: 'small',
}}
/>
Copy link
Member

Choose a reason for hiding this comment

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

If you do not wrap the <Form.Item noStyle shouldUpdate={...}>, this part will not re-render when vFolderName is changed. Please check my previous comment again.

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.

LGTM, but I'll merge this after reviewing the top of the stack. #2447

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:52 AM EDT: @yomybaby merged this pull request with Graphite.

@yomybaby yomybaby merged commit 2c8a16d into main May 31, 2024
11 checks passed
@yomybaby yomybaby deleted the feature/refactor-model-storage-mount-in-model-service-launcher branch May 31, 2024 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:i18n Localization area:ux UI / UX issue. impact:breaking Breaking or highlighted changes. size:XL 500~ LoC type:enhance Add new features type:refactor Refactoring current implementation. urgency:blocker IT SHOULD BE RESOLVED BEFORE DISTRIBUTING urgency:5 It is imperative that action be taken right away.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants