containers / podman-desktop-extension-ai-lab

Work with LLMs on a local environment using containers
https://podman-desktop.io/extensions/ai-lab
Apache License 2.0
182 stars 39 forks source link

feat: start instructlab session #1849

Closed axel7083 closed 1 month ago

axel7083 commented 1 month ago

What does this PR do?

After discussion happening in https://github.com/containers/podman-desktop-extension-ai-lab/issues/1617, here is the frontend implementation for the New Instruct Lab session page

Screenshot / video of UI

https://github.com/user-attachments/assets/66421239-8a15-426f-8f04-d1dbdb491e3e

ℹ️ I took inspiration from the Play Kubernetes YAML page for the radio selection. It give better consistency has it was the only place with similar design

image

What issues does this PR fix or reference?

Fixes https://github.com/containers/podman-desktop-extension-ai-lab/issues/1617

How to test this PR?

jeffmaury commented 1 month ago

I'm questioning the Skills or Knowledges option, I don't see where in InstructLab this is a requirement.

MariaLeonova commented 1 month ago

@axel7083 looks good - I'm glad to see this coming together! Couple minor things I'd like to request:

@jeffmaury let's check with the InstructLab team - do you know who would be the best person to contact there for more info?

gastoner commented 1 month ago

I would change the "not selected" knowledge/skill faCircleCheck to some blank one or just some circle, because since we have two faCircleCheck, it seems to me like you have both enabled and you are just adding skills/knowledge to both of them in a same time. Or use just radio buttons image

MariaLeonova commented 1 month ago

I would change the "not selected" knowledge/skill faCircleCheck to some blank one or just some circle, because since we have two faCircleCheck, it seems to me like you have both enabled and you are just adding skills/knowledge to both of them in a same time. Or use just radio buttons image

@axel7083 I second that - the pattern I added in the mock already exists in Podman Desktop UI, soo I think that element can be reused. Screenshot 2024-10-08 at 8 35 20

jeffmaury commented 1 month ago

@axel7083 looks good - I'm glad to see this coming together! Couple minor things I'd like to request:

* Can the `+add knowledge/skill to use` button be left-aligned as the rest of the UI elements, since there's no input field next to it?

* The suggestion was for section to be called "Fine-tune with InstructLab" - I see the UI is structured a bit differently from [the mock](https://private-user-images.githubusercontent.com/14108590/373563894-91dd53f4-423a-41f7-be61-0d7fa16e6a86.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjgzMzI5ODMsIm5iZiI6MTcyODMzMjY4MywicGF0aCI6Ii8xNDEwODU5MC8zNzM1NjM4OTQtOTFkZDUzZjQtNDIzYS00MWY3LWJlNjEtMGQ3ZmExNmU2YTg2LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDEwMDclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQxMDA3VDIwMjQ0M1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTJkNDViMDZmNzBlODI0YjEzY2I2YWFhOTgyZDJjZTU1ZmU0YzhjNDQ3OGY3NmZlMjQzYjQzZDlmZjgzNDE3NTcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.dI_1_cGZqQVZ66pbnOcMQDHTeRSJcedxHvQY155_GCg), but I think it will benefit from this title.

* On that note, let's agree on the consistent spelling: my suggestion is Fine-tune and fine-tuning, as on [Hugging Face](https://huggingface.co/docs/transformers/en/training#fine-tune-a-pretrained-model)

@jeffmaury let's check with the InstructLab team - do you know who would be the best person to contact there for more info?

Best is to ask on one of their Slack channels

MariaLeonova commented 1 month ago

Folks, the InstructLab beta UI is out, and they have nice short descriptions for knowledge and skills contributions, shall we consider adding those to fields helper text?

🌟 Knowledge in InstructLab is represented by question and answer pairs that involve facts, data, or references. 🌟 Skills are performative. When you create a skill for the model, you are teaching it how to do something: "write me a song," "rearrange words in a sentence" or "summarize an email." 🌟

MariaLeonova commented 1 month ago

Adding a comment from Máirín Duffy on the InstructLab side:

for laptop training, full pipeline, right now - you can train knowledge or skills. not both in a training run.

axel7083 commented 1 month ago

reviewing that PR I noticed that we have a store to get the modelInfos and a method on the studioClient studioClient.getModelsInfo and the store delegates to the method, so I'm finding this confusing

We cannot subscribe to the studioClient.getModelsInfo. We could argue that we could get those value in the onMount, and there is little chance that the models will change on this page, but this is not the choice made in other forms, for reactivity purpose.

Like here, in the component NewInstructlabSession, we import both studioClient and the store but there are overlap between the two

The store is automatically updated every time the backend notify the webview through the Messages.MSG_NEW_MODELS_STATE channel

https://github.com/containers/podman-desktop-extension-ai-lab/blob/c165dbbb689773b2c264dbbb311a9820357d7da2/packages/frontend/src/stores/modelsInfo.ts#L24

This is why we have two components here, they don't overlap has the store has an auto update logic based on event, that we do not have with a simple studioClient.getModelsInfo.

We import the studioClient to access to two method studioClient.openDialog (open the explorer to select a folder) and studioClient.openUrl to open in the browser the documentation url.

In AI Lab we do not declare global method like in Podman Desktop, where all the communication between the renderer and the preload are made through the window.* object so we need to import the studioClient to be able to interact with the backend.

benoitf commented 1 month ago

This is why we have two components here, they don't overlap has the store has an auto update logic based on event, that we do not have with a simple studioClient.getModelsInfo.

From consumer side, you've two different equivalent as it's from the same source. Exposure is changing. Someone onboarding into the project might think it's more convenient to import only the client and then call client.getModelsInfo() But then, why I should have access to something not being reactive ? Also you can use get(modelsInfo) to get the value for one time. (no subscribe/unsubscribe)

I'm also offtrack of this PR, I don't require any change on this PR about that, but probably need to have a follow-up issue

my point is more : it looks that there is something more 'internal' to grab data from the backend side but client should never consume this endpoint directly. And then there are methods exposed to the client that he can use. While here, it's a mix of two. Because if you need to ask something like 'publicly'

And stores being deprecated over $state() it could be revisited at that point.

axel7083 commented 1 month ago

my point is more : it looks that there is something more 'internal' to grab data from the backend side but client should never consume this endpoint directly. And then there are methods exposed to the client that he can use. While here, it's a mix of two. Because if you need to ask something like 'publicly'

I get your point, and totally understand it, there is clearly room for improvement, especially with the svelte 5 new requirements.

I'm also offtrack of this PR, I don't require any change on this PR about that, but probably need to have a follow-up issue

I agree, I opened https://github.com/containers/podman-desktop-extension-ai-lab/issues/1883 to continue this conversation, as I think this is a valid point, and I would welcome change to improve this system.

I want to precise however that this system has been design and put together the first days of the hackathons in January, and has not been touch much since.

axel7083 commented 1 month ago

Can we make the session name optional ?

Feel reasonable to require a name cc @MariaLeonova

MariaLeonova commented 1 month ago

Can we make the session name optional ?

Feel reasonable to require a name cc @MariaLeonova

@jeffmaury why do you suggest to make it optional?

The idea there was for people to be able to easily find their session in a list later. That's something that's done in the IBM Cloud flow, as well. I suppose it can be pre-filled with an automatically generated name, if we don't want users to have to type anything in?

axel7083 commented 1 month ago

That's something that's done in the IBM Cloud flow, as well.

Personal preference to have it mandatory

jeffmaury commented 1 month ago

We don't require it for playgrounds and services

MariaLeonova commented 1 month ago

@jeffmaury ultimately it's up to you, as I don't have enough background info on user behaviour in playgrounds and services. Just as long as users are allowed to change the name there to something that makes sense to them - I think in this case it's important to give them control over their session list. Which seems to be the case for playgrounds, at least.

Screenshot 2024-10-14 at 9 14 46
jeffmaury commented 1 month ago

@jeffmaury ultimately it's up to you, as I don't have enough background info on user behaviour in playgrounds and services. Just as long as users are allowed to change the name there to something that makes sense to them - I think in this case it's important to give them control over their session list. Which seems to be the case for playgrounds, at least. Screenshot 2024-10-14 at 9 14 46

Yes I agree session name should stay

axel7083 commented 1 month ago

The +add button shouldn't be centered, if we are going with this pattern.

image

Updated :rocket:

slemeur commented 1 month ago

@MariaLeonova: I have a couple of UX feedback.

When the box is not selected / no files added It really feels that we are duplicating the labels. There is "Add Knowledge" title and below "Add Knowledge to use", that feels redundant to me. I'd also callout that the "Add Knowledge to use" does not look like a button and that does not feel very intuitive.

When a file has been added The description of what the section is about feels not at the right location. That should probably be before the action.

MariaLeonova commented 1 month ago

Thank you for providing this feedback!

@MariaLeonova: I have a couple of UX feedback.

When the box is not selected / no files added It really feels that we are duplicating the labels. There is "Add Knowledge" title and below "Add Knowledge to use", that feels redundant to me. I'd also callout that the "Add Knowledge to use" does not look like a button and that does not feel very intuitive.

I agree! It used to say 'add another knowledge file' in the mock. New IL Session Form- 4 _ 10 (1)

When a file has been added The description of what the section is about feels not at the right location. That should probably be before the action.

@axel7083 can you please add a screenshot of what it looks like when there are 2 files added already? We might want to shuffle things a bit - the addition of +add more knowledge/skills button has rearranged elements in the form.

axel7083 commented 1 month ago

image

@MariaLeonova @slemeur can we iterate in an dedicated issue, and merge this one ? This page is not even visible to the user, nor has any implementation logic, I would rather focus on https://github.com/containers/podman-desktop-extension-ai-lab/issues/1851 and have something working to experiment with, rather than having the perfect page without anything behind.

MariaLeonova commented 1 month ago

@axel7083 I am not opposed to opening a follow-up issue for design improvement, so we don't have to go back and forth and can change all at once, if you'd rather merge this now.