ollama: smaller cuda plus minor simplifications #324659
Conversation
|
@ofborg test ollama |
There was a problem hiding this comment.
@abysssol how'd you feel about inlining some of the let-in's as well? And maybe nixfmt-ing the expression?
If you prefer I can leave out the commit that moves the attributes around and you can handle this yourself
There was a problem hiding this comment.
I pushed a few commits, give them a look, tell me what you think.
I don't want to inline vendorHash because I want to keep version, src, vendorHash, and llamacppPatches together to make updating the ollama version easier, and make it harder to forget or miss something.
There was a problem hiding this comment.
apple_sdk_11_0
Is it necessary to select the particular release? Iirc the unqualified apple_sdk had some logic that was supposed to make it the safe choice
There was a problem hiding this comment.
I used apple_sdk_11_0 because of some problem with a missing framework in it, and this comment recommended using apple_sdk_11_0 instead of apple_sdk, which seemed to work.
However, I think this was only a problem on platforms other than darwin, so switching to apple_sdk would probably still work right, though may cause warnings or errors with ofBorg's package evaluation.
There was a problem hiding this comment.
Oh. If that's causing problems only on some platforms, and especially on non-darwin platforms which should be unaffected - that might be something to bring up with the darwin team. I'll confirm whether there's an issue first though
pkgs/by-name/ol/ollama/package.nix
Outdated
| ] ++ lib.optionals stdenv.isDarwin | ||
| metalFrameworks; | ||
| ]; |
There was a problem hiding this comment.
From what I remember, the metal frameworks needed to be in both nativeBuildInputs and buildInputs for ollama to build right on darwin/macos. See this comment after I tried moving them from nativeBuildInputs to buildInputs.
If you really want to leave this in, we'll need to verify that ollama works right on darwin, but I think this change should be removed.
There was a problem hiding this comment.
I think I should read the whole thread before I move on
github-actions
bot
added
the
6.topic: nixos
nixos/tests/ollama.nix
Outdated
| @@ -32,6 +32,7 @@ in | |||
| }; | |||
|
|
|||
| cuda = { ... }: { | |||
| nixpkgs.config.allowUnfree = true; | |||
There was a problem hiding this comment.
This will break setups that pass pkgs to lib.nixosSystem. There is an assert in that module that fails in this case. I'd suggest removing this line.
There was a problem hiding this comment.
I don't understand why this is a problem. Can you explain in more detail how lib.nixosSystem would fail because a test vm enabled allowUnfree?
There is an assert in that module that fails in this case.
What assert in what module?
Also, why pass pkgs to lib.nixosSystem? I don't see what the purpose of that is, or what that might accomplish.
This line was added to allow ofborg to directly run ollama's nixos test (ie @ofborg test ollama) since it previously failed to evaluate it, complaining about the unfree cuda dependencies. Do you know any way to solve this problem without causing errors when passing pkgs to lib.nixosSystem?
There was a problem hiding this comment.
This will break setups that pass pkgs to lib.nixosSystem.
This is a nixos test, not nixos module, so it itself is responsible for calling nixosSystem?
This line was added to allow ofborg to directly run ollama's nixos test
I'm not sure whether we want the ofborg to test unfree things. I think so far it hasn't (except by accident). That said, we always wanted an ofborg to do that
There was a problem hiding this comment.
I'm not sure whether we want the ofborg to test unfree things.
Why not?
we always wanted an ofborg to do that
What's wrong with the current ofborg?
There was a problem hiding this comment.
I overlook the fact that this file is actually a test 🤦
But in the main module that would be a problem. In my config, for example, I instantiate a outer pkgs then pass to all my machines and home-manager. Easier to add stuff that cascades into the machines.
There was a problem hiding this comment.
What's wrong with the current ofborg?
It's just the risk aversity regarding the official infra and the unfree. Another issue might be, ofborg won't be able to substitute cuda stuff using the official cache.
That said, Ofborg's build outputs aren't redistributed, and we could in principle make Ofborg use the nix-community cache, so I'm not sure if there's any policy or decision regarding Ofborg specifically. And eitherways if there is one, now's a good time to reconsider.
But yeah I think this change involves more parties that must be made aware of the change and consulted with...
There was a problem hiding this comment.
ofborg won't be able to substitute cuda stuff using the official cache.
Again, why not? Not all unfree licenses restrict redistribution of binaries or other build artifacts. Does cuda?
But yeah I think this change involves more parties that must be made aware of the change and consulted with...
Yeah, I agree. I don't actually know who any of those people are, though. Do you?
There was a problem hiding this comment.
Again, why not? Not all unfree licenses restrict redistribution of binaries or other build artifacts. Does cuda?
Ofborg won't be able to substitute because the official hydra doesn't build unfree, and the official hydra doesn't build unfree because last time the proposal met a quite a bit of opposition (e.g. https://discourse.nixos.org/t/petition-to-build-and-cache-unfree-packages-on-cache-nixos-org/, but there were discussions before and after). Not that this is set in stone, but also this is probably OK if we can rely on the nix-community hydra instead. Ofborg and CI are yet to be figured out
Yeah, I agree. I don't actually know who any of those people are, though. Do you?
One could start by (asking, again, advice from the broader contributor community, the infra team, the foundation people, and) posting a proposal on discourse, arguing that there's no legal threat (is there?) in just having Ofborg test the unfree stuff without redistribution (is there?) so why don't we just enable it.
If that meets opposition, we look for an opportunity to run (or co-host) a separate ofborg-like CI instance that could be integrated with github...
There was a problem hiding this comment.
This all sounds to me like a pretty significant project, and much more of a hassle than I feel like getting into at the moment. I may feel differently at some point in the future, but for now I'll be putting anything related to cuda/unfree licensing on hold.
There was a problem hiding this comment.
Also cf NixOS/ofborg#644
abysssol
force-pushed
the
refactor/ollama
branch
from
9b3dd3c to
c84462e
Compare
github-actions
bot
removed
the
6.topic: nixos
ofborg
bot
added
the
2.status: merge conflict
abysssol
force-pushed
the
refactor/ollama
branch
from
c84462e to
ef73227
Compare
abysssol
removed
the
2.status: merge conflict
wegank
added
the
2.status: merge conflict
|
What's the status of this PR ? |
|
@drupol this was a refactoring but derived from a much older version of the package, I guess you're right to close it |
|
Yes, and since I had no feedback, I closed it. |
Description of changes
(cf commit messages)
CC @yshui @abysssol
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.