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

example/llm: minor refactor #227

Merged
merged 4 commits into from
Jun 14, 2024
Merged

example/llm: minor refactor #227

merged 4 commits into from
Jun 14, 2024

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Jun 14, 2024

Hi!

I separated everything into small commits for easier review.

Let me know what you think!

@shivaraj-bh
Copy link
Member

The purpose behind adding the model here was to ensure the user can try out inference on a model immediately after Open WebUI launches. If 4GB is too much, we can go with something smaller?

@drupol
Copy link
Contributor Author

drupol commented Jun 14, 2024

Actually, I removed it because open-webui is able to download it through the UI.

I believe that the goal of this example is to show users that open-webui works immediatelly, and they can configure ollama and the models to use through the web interface.

Copy link
Member

@srid srid left a comment

Choose a reason for hiding this comment

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

Please remove the formatting/ indentation changes as they are not necessary (nor in line with the rest of the project tree) and makes review more difficult.

open-webui is able to download it through the UI.

How straightforward a process is that? It looks like you have to go to settings -> Admin Panel -> Models and enter the model tag?

image

@drupol
Copy link
Contributor Author

drupol commented Jun 14, 2024

Please remove the formatting/ indentation changes as they are not necessary (nor in line with the rest of the project tree) and makes review more difficult.

That's the reason why I separated everything by commit. There's a standardised formatting now in Nix, are you sure you don't want it here?

open-webui is able to download it through the UI.

How straightforward a process is that? It looks like you have to go to settings -> Admin Panel -> Models and enter the model tag?
image

Yep, as simple as that indeed, very handy !

@srid
Copy link
Member

srid commented Jun 14, 2024

example/llm: reformat with nixfmt-rfc-style

Actually I just saw this. @drupol I'd appreciate if you open different PRs for these. We use nixpkgs-fmt. What is nixfmt-rfc-style? Don't we have a CI check for formatting (I'd expect it to fail on this PR, but it doesn't @shivaraj-bh). Again, best to discuss this in its own PR.

@drupol
Copy link
Contributor Author

drupol commented Jun 14, 2024

OK !

@srid
Copy link
Member

srid commented Jun 14, 2024

I'd say there should be three PRs,

  • Removing the model download (it being a non-trivial change)
  • Formatting change
  • Rest of the commits in here

@srid
Copy link
Member

srid commented Jun 14, 2024

  • Rest of the commits in here

Regarding which:

image

No idea why this was removed.

@drupol
Copy link
Contributor Author

drupol commented Jun 14, 2024

  • Rest of the commits in here

Regarding which:
image

No idea why this was removed.

IMHO, I believe it's better to "show" the user of this project that data are saved within the same directory as the flake. He's free to change the dataDir of course.

To be honest that was the first thing that was kinda weird when I first tested this project. I expected the stateful data to be saved within the same directory and it was not the case.

@srid
Copy link
Member

srid commented Jun 14, 2024

@drupol Would you like to join our Zulip? We are discussing a couple of your changes. I've responded to your last comment here as well:

https://nixos.zulipchat.com/#narrow/stream/414011-services-flake/topic/Require.20opinion.20on.20.60example.2Fllm.60/near/444705940

@shivaraj-bh shivaraj-bh merged commit 8010b5a into juspay:main Jun 14, 2024
4 checks passed
@drupol drupol deleted the fix-open-webui-example branch June 14, 2024 14:45
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.

3 participants