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

ollama: 0.1.31 -> 0.1.38 #312608

Merged
merged 2 commits into from
May 19, 2024
Merged

ollama: 0.1.31 -> 0.1.38 #312608

merged 2 commits into from
May 19, 2024

Conversation

abysssol
Copy link
Contributor

@abysssol abysssol commented May 18, 2024

Description of changes

Ollama's rocm build was broken in more recent ollama releases, but this has now been (at least partially) fixed by adding rocmPath to the wrapper's LD_LIBRARY_PATH to allow runtime detection, adding a patch to disable a check that exits compilation early, and enabling tensileSepArch and tensileLazyLib for rocblas. These fixes were discovered thanks to @volfyd in discussions on pr #309330.

Testing of the new version with acceleration = "rocm"; enabled is highly appreciated. The following command should be able to build it when run in the root of the nixpkgs repository.

nix-build --expr '(import ./. {}).ollama.override { acceleration = "rocm"; }'

Tinydolphin is a small model that can download quickly, if you don't already have any models.

./result/bin/ollama serve &>./ollama-debug.log &
./result/bin/ollama run tinydolphin && kill %%

Kill works differently in fish:

./result/bin/ollama serve &>./ollama-debug.log &
./result/bin/ollama run tinydolphin && kill (jobs -lg)

Due to uncertainty about the completeness of this fix, this won't be merged until after the next stable release, which should be soon (see #303285).

Changelog

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@abysssol abysssol requested a review from drupol May 18, 2024 08:07
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

LGTM ! Feel free to merge when CI is done.

Copy link
Member

@surfaceflinger surfaceflinger left a comment

Choose a reason for hiding this comment

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

Works for me with rocm, thanks!

@drupol
Copy link
Contributor

drupol commented May 19, 2024

I think we can safely ignore aarch64-darwin, it's definitely too long and counter productive. Let's merge it, @abysssol go for it!

@abysssol
Copy link
Contributor Author

@drupol Since I'm not entirely confident that 0.1.38 will have complete compatibility with 0.1.31 as far as rocm is concerned (some specific gpus could possibly lose support, for example), I was planning to wait until after the stable release of nixos 24.05, to prevent the possibility of a breaking change getting into 24.05. This should only take a few days, as according to #303285 the release-24.05 branch is planned to be created on 2024-05-22, and new merges into master won't get into 24.05 after that, if my understanding is correct.

If you think I specifically should try to get this pr into the next stable release, could you explain your reasoning? I'm not particularly comfortable with the possibility of merging a breaking change into the next stable release at the last moment, even if it's relatively unlikely. Since this isn't a particularly urgent change, it seems perfectly reasonable to wait a few days and merge it into the next unstable, where breaking changes are more tolerable. And now that I've gotten committer privileges, I won't have to bother you to actually merge it when I think it's ready, since I have your approval. It can always be backported later, maybe after a month with no issues?

@drupol
Copy link
Contributor

drupol commented May 19, 2024

If this PR might introduce issues, then it's indeed better to wait, no worries.

And now that I've gotten committer privileges, I won't have to bother you to actually merge it when I think it's ready, since I have your approval. It can always be backported later, maybe after a month with no issues?

👍

@nonetrix
Copy link
Contributor

nonetrix commented May 19, 2024

With the current version of Ollama already in Nixpkgs it's broken already for me, doesn't do any VRAM allocation or anything. It worked before, but doesn't now for some reason. It's just like that for some reason seemingly though, it will work one moment then not the next. It at least builds though, not sure what would cause it to act that way since I don't think any recent changes have been made to the Ollama nix file

@abysssol
Copy link
Contributor Author

@nonetrix How long has it been having these problems, if you remember? I believe the last change to the ollama package was made in #303708, which has been more than five weeks now. Do you know if that worked at release or not? I just tested 0.1.31 and also found that it doesn't use my gpu, claiming it to be unsupported. I wonder if I never tested 0.1.31 properly, or if some change to rocblas could have caused a transitive change in ollama's behavior.

@drupol do you think it's worth merging now despite the possibility of a problem? since it seems likely that this could fix existing issues with the current version of ollama, which I hadn't considered, and may be more widespread than I realized. I'm no longer entirely confident in my lack of confidence.

@drupol
Copy link
Contributor

drupol commented May 19, 2024

IMHO, yes I would merge this.

@abysssol abysssol marked this pull request as ready for review May 19, 2024 20:32
@abysssol abysssol merged commit 82193f4 into NixOS:master May 19, 2024
33 of 34 checks passed
@abysssol abysssol deleted the ollama-update-0.1.38 branch May 19, 2024 20:33
@abysssol abysssol restored the ollama-update-0.1.38 branch May 20, 2024 13:30
@abysssol abysssol deleted the ollama-update-0.1.38 branch May 20, 2024 13:31
@nonetrix
Copy link
Contributor

nonetrix commented May 22, 2024

Got the update, working great for me so far GPU and all! 0.1.39 would be nice soon though, phi3 medium is broken because llama cpp version is not updated yet. Also, maybe seems maybe faster now? Which is nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants