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: adding LocalRepositoryRegistry #298

Merged
merged 12 commits into from
Feb 15, 2024

Conversation

axel7083
Copy link
Contributor

@axel7083 axel7083 commented Feb 14, 2024

What does this PR do?

Simply add a store containing the information of where are stored the checkout repository. This store will be required to be able to have #27

This store could be used in the future for #234

What issues does this PR fix or reference?

Fixes #250

How to test this PR?

  • Unit tests

@axel7083 axel7083 requested a review from a team as a code owner February 14, 2024 10:31
@axel7083 axel7083 force-pushed the feature/local-repositories-store branch from 784e8c7 to 6ceac8b Compare February 14, 2024 12:30
@axel7083 axel7083 requested a review from lstocchi February 14, 2024 12:31
@lstocchi
Copy link
Contributor

I'm trying to figure out which is the final result for this. I looked at #250 but i didn't get the purpose. Let's say you know which recipe repos the user downloaded, what are you going to do with that info? Is there any new page/icon/message you're going to show?

@axel7083
Copy link
Contributor Author

I'm trying to figure out which is the final result for this. I looked at #250 but i didn't get the purpose. Let's say you know which recipe repos the user downloaded, what are you going to do with that info? Is there any new page/icon/message you're going to show?

@lstocchi the open in VSCode button see #27

constructor(private webview: Webview) {}

register(localRepository: LocalRepository): podmanDesktopApi.Disposable {
this.repositories.set(localRepository.path, localRepository);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe using the recipe.id as key?
Now we use a static path but let's say the user could customize it you could end up with two values for the same recipe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like it is easy to have multiple version of the same recipeId. Having the path as key ensure uniqueness, what if the user decide to clone twice the same recipe in different directory (two different branch let's say)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the point of having multiple cloned repo of the same recipe. If you are working on an edited version it's like you want to test it, if it gets broken you would update the code/reset it in your IDE.

If you open the recipe page you're targeting a specific version, so the open to vscode should be related to that one. If you have multiple instances of the same recipe in the store, how would you pick the one to use? Or everytime the user click on run application you make him decide which one to run?

Copy link
Contributor Author

@axel7083 axel7083 Feb 14, 2024

Choose a reason for hiding this comment

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

I see your point, but if you select a local repository which has no recipe id ?

For example, you are creating yourself a repository, and want to open in inside the IA-Studio, at some point you will only have a ai-studio.yaml and it has no concept of recipe here.

See https://github.com/redhat-et/locallm/blob/main/chatbot/ai-studio.yaml

We want people to be able to use their own code/repo without forcing them to be inside the catalog. I mention #234 which is the use case

We can imagine in the future, the local repository (user configured) and then nothing to do with recipe, what would be their id then?

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, if you import a local/remote repo like the one mentioned (so the chatbot), it is going to create a new item in the recipe catalog page (maybe under a local section). By looking at the ai-studio.yaml you have all infos to make it work.
The model backend tells you which are the model type supported and we can use that one to fill the models tab. Then to run the application we have the ai-studio.yaml file.

BTW this is just the way i see it.
@jeffmaury WDYT? Any idea how it should work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree we could show the local projects/repository manually added by the user to the recipe page, under as mention some local sections.

But they are not recipes, they are a local project/folder/repository, and what's unique about them, their path.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could use some fake id to differentiate them and use the same logic to keep 1-1 recipe-source.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's really a matter if we want to store multiple sources per recipe or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would we generate some fake ids, when we could have the path as id ?

at some point we will have to save the content of the LocalRepositoryRegistry to a file, as the projects/folder/repositories imported by the user would have to be still their after a reboot, I would prefer avoiding generating some random values to use that as id, when we could avoid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's really a matter if we want to store multiple sources per recipe or not.

i would argue no, as you mention, this is probably not something expected, I am more worried about the behavior to adopt for external folder/projects/repositories

packages/frontend/src/stores/localRepositories.ts Outdated Show resolved Hide resolved
@axel7083 axel7083 force-pushed the feature/local-repositories-store branch from 7f1e468 to d551e48 Compare February 15, 2024 13:12
@axel7083 axel7083 requested a review from lstocchi February 15, 2024 13:12

constructor(private webview: Webview) {}

register(localRepository: LocalRepository): Disposable {
Copy link
Contributor

Choose a reason for hiding this comment

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

This registry will be available the time you are in the same instance of the AI Studio app. But how will you build it again when you restart the app (either Podman Desktop or the extension?). Will you have the pair path/recipeId somewhere to be able to rebuild this "database"?

Copy link
Contributor Author

@axel7083 axel7083 Feb 15, 2024

Choose a reason for hiding this comment

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

@feloy yes this is the goal in the future. For now we do not support adding manually folder/projects/repositories provided by the user. But we would at some point, and we want a place to store that information see #234

Today this is simply in memory, but yes in the future we would have to store that information somewhere on disk. But this is not part of this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

The solution for the models on disk is to use the modelId as directory name and to rely on it, so we can rebuild this database by scanning the directories in the dedicated directory. Could it be applied here also?

If we don't want to handle the manually created repositories for now, we will have only one repository on disk for each recipe, right? Even if I switch the model, if I understand correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The solution for the models on disk is to use the modelId

I am not talking about the models id here, but folders containing ai-studio.yaml file

If we don't want to handle the manually created repositories for now, we will have only one repository on disk for each recipe, right? Even if I switch the model, if I understand correctly.

Yes exactly, for now we will only have the cloned repository inside the LocalRepositoryRegistry

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not talking about the models id here, but folders containing ai-studio.yaml file

Yes, I understand, but for me the problem of getting the path of the model's directory is the same as getting the recipy/repository's directory. The reason why I'm comparing both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sure

Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
@axel7083 axel7083 force-pushed the feature/local-repositories-store branch from 4197dbe to 5f5d6e2 Compare February 15, 2024 15:04
Copy link
Contributor

@feloy feloy left a comment

Choose a reason for hiding this comment

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

lgtm

@axel7083 axel7083 enabled auto-merge (squash) February 15, 2024 16:29
@axel7083 axel7083 merged commit d6b791d into containers:main Feb 15, 2024
3 checks passed
mhdawson pushed a commit to mhdawson/podman-desktop-extension-ai-lab that referenced this pull request Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

persistence : we need to know when starting which recipe are already pulled
3 participants