postgresqlPackages.pgvecto-rs: init at 0.2.1 #281192
Conversation
pkgs/servers/sql/postgresql/ext/pgvecto-rs/0001-read-clang-flags-from-environment.diff
Show resolved
Hide resolved
|
It looks like v0.2.0 has been released and that Immich is moving to that version of pgvecto.rs in the next release: immich-app/immich#6785 I'll be working tomorrow to get this ready in time for the release! |
diogotcorreia
force-pushed
the
pgvecto.rs
branch
from
76f2806 to
77122ea
Compare
diogotcorreia
changed the title
ofborg
bot
added
8.has: package (new)
diogotcorreia
force-pushed
the
pgvecto.rs
branch
from
77122ea to
fe4cc64
Compare
| # avoid future incompatibility. | ||
| # See https://docs.pgvecto.rs/developers/development.html#environment, step 4 | ||
| rustPlatform' = rustPlatform // { | ||
| inherit (callPackage ../../../../../build-support/rust/hooks { |
There was a problem hiding this comment.
Relative paths are not part of a package's interface. We'll have to find a different solution.
There was a problem hiding this comment.
You're absolutely right. I'll investigate alternative solutions
There was a problem hiding this comment.
There doesn't seem to be a way to override it. This is what I've tried:
nix-repl> pkgs.rustPlatform.bindgenHook.override { clang = pkgs.clang_17; }
error:
… while calling a functor (an attribute set with a '__functor' attribute)
at «string»:1:1:
1| pkgs.rustPlatform.bindgenHook.override { clang = pkgs.clang_17; }
| ^
… while calling a functor (an attribute set with a '__functor' attribute)
at /home/dtc/documents/vcs/nixpkgs/lib/customisation.nix:104:43:
103| # Re-call the function but with different arguments
104| overrideArgs = mirrorArgs (newArgs: makeOverridable f (overrideWith newArgs));
| ^
105| # Change the result of the function call by applying g to it
(stack trace truncated; use '--show-trace' to show the full trace)
error: function 'anonymous lambda' called with unexpected argument 'clang'
at /home/dtc/documents/vcs/nixpkgs/pkgs/build-support/rust/hooks/default.nix:92:32:
91|
92| bindgenHook = callPackage ({}: makeSetupHook {
| ^
93| name = "rust-bindgen-hook";
nix-repl> (pkgs.makeRustPlatform {rustc = pkgs.rustc; cargo = pkgs.cargo; clang = pkgs.clang_17;}).bindgenHook.clang
«derivation /nix/store/j16048gsic45ijdpw82172pfxdz9nw2w-clang-wrapper-16.0.6.drv»
For now, I'll setup a similar script just for this package that supports pinning the clang version, but this should be revisited at a later date to allow overriding this.
There was a problem hiding this comment.
This looks like a bug. It can be fixed with this:
diff --git a/pkgs/development/compilers/rust/make-rust-platform.nix b/pkgs/development/compilers/rust/make-rust-platform.nix
index e22cb6d594af..6ed724aae821 100644
--- a/pkgs/development/compilers/rust/make-rust-platform.nix
+++ b/pkgs/development/compilers/rust/make-rust-platform.nix
@@ -1,4 +1,4 @@
-{ lib, buildPackages, callPackage, cargo-auditable, stdenv, runCommand }@prev:
+{ lib, buildPackages, callPackage, callPackages, cargo-auditable, stdenv, runCommand }@prev:
{ rustc
, cargo
@@ -34,7 +34,7 @@ rec {
};
# Hooks
- inherit (callPackage ../../../build-support/rust/hooks {
+ inherit (callPackages ../../../build-support/rust/hooks {
inherit stdenv cargo rustc;
}) cargoBuildHook cargoCheckHook cargoInstallHook cargoNextestHook cargoSetupHook maturinBuildHook bindgenHook;
}There was a problem hiding this comment.
I'll investigate that later then, thanks :)
Should this be made in another PR, or does it make sense to include it in this one?
There was a problem hiding this comment.
It makes sense to include here; it's a requirement for this to work. If you needed to refactor something else for this to work, you'd also put it here.
If this was to stall because of something related to pgvector directly, I'd advocate for spinning the refactors out into their own PRs but we'll cross that bridge when we get there.
There was a problem hiding this comment.
Great, it seems to work fine in the repl. I've tested it by overriding clang to 17 and having it fail the build.
I'm just finishing building the package to make sure everything works fine, and then I'll push.
This will likely trigger a lot of package rebuilds in the CI, but we'll see 🙃
There was a problem hiding this comment.
Relative paths are not part of a package's interface. We'll have to find a different solution.
Sorry, I didn't understand that comment, and I'm curious about this... Why is the relative path a problem? Also, for example, pkgs/os-specific/darwin/apple-sdk-11.0/default.nix does something similar:
rustPlatform = pkgs.makeRustPlatform {
inherit (pkgs.darwin.apple_sdk_11_0) stdenv;
inherit (pkgs) rustc cargo;
} // {
inherit (pkgs.callPackage ../../../build-support/rust/hooks {
inherit (pkgs.darwin.apple_sdk_11_0) stdenv;
inherit (pkgs) cargo rustc;
clang = mkCc pkgs.clang;
}) bindgenHook;
};
There was a problem hiding this comment.
Relative paths are not guaranteed to remain stable.
People who reorganise one file in nixpkgs should not be expected to check and adjust relative paths in all other files.
This is relevant as we'll soon see the great RFC140 migration.
Now that the hooks override is fixed, the the apple SDK should use this style aswell as it doesn't have this obvious issue. (It may still have less obvious issues.)
diogotcorreia
force-pushed
the
pgvecto.rs
branch
from
fe4cc64 to
c8672e2
Compare
github-actions
bot
added
the
6.topic: nixos
|
Can you add the source files from |
|
@PhilippWoelfel that was also in my todo list as well, yes :) I'm not sure if I'm going to be able to finish the remaining problems this with PR this weekend, but I'll do my best 😅 |
diogotcorreia
force-pushed
the
pgvecto.rs
branch
from
c8672e2 to
ff6e8ff
Compare
|
I think it should be ready now :) 🤞 Changes I've made:
Since I'm force pushing, here's the diff: https://bin.diogotc.com/xuzecylyju.diff |
This comment was marked as off-topic.
This comment was marked as off-topic.
diogotcorreia
requested review from
Mic92,
zowoq,
winterqt and
figsoda
as code owners
| rustPlatform' = rustPlatform // { | ||
| bindgenHook = rustPlatform.bindgenHook.override { inherit clang; }; | ||
| }; |
There was a problem hiding this comment.
I'd prefer a full makeRustPlatform call.
There was a problem hiding this comment.
I tried that, but it seems to not be overriding as expected.
nix-repl> (pkgs.makeRustPlatform {rustc = pkgs.rustc; cargo = pkgs.cargo; clang = pkgs.clang_17;}).bindgenHook.clang
«derivation /nix/store/j16048gsic45ijdpw82172pfxdz9nw2w-clang-wrapper-16.0.6.drv»
Am I doing it wrong?
There was a problem hiding this comment.
Ah, makeRustPlatform doesn't take a clang arg but does accept arbitrary args, so it doesn't throw an error to do this.
I don't see another option short of refactoring makeRustPlatform then.
I really don't know how this all works together; doesn't rust internally depend on clang aswell? Shouldnt'y you need to override that aswell?
I'd like input of the rust people on this. It appears they were pinged already.
There was a problem hiding this comment.
This is what we do for firefox:
There was a problem hiding this comment.
Not sure if it also applies to this.
There was a problem hiding this comment.
@Mic92 Thanks for you help, but it seems that it still doesn't work. Let me know if I'm using this wrong:
nix-repl> (pkgs.makeRustPlatform { inherit (pkgs) rustc cargo; stdenv = pkgs.overrideCC pkgs.llvmPackages.stdenv (llvmPackages.stdenv.override { cc = pkgs.clang_17; }); }).bindgenHook.clang
«derivation /nix/store/c9ym6jk1pf0bx62j29g51k00pp16zf9l-clang-wrapper-16.0.6.drv»
nix-repl> (pkgs.makeRustPlatform { inherit (pkgs) rustc cargo; stdenv = pkgs.overrideCC pkgs.llvmPackages.stdenv.override { cc = pkgs.clang_17; }; }).bindgenHook.clang
«derivation /nix/store/c9ym6jk1pf0bx62j29g51k00pp16zf9l-clang-wrapper-16.0.6.drv»
nix-repl> (pkgs.makeRustPlatform { inherit (pkgs) rustc cargo; stdenv = pkgs.llvmPackages.stdenv.override { cc = pkgs.clang_17; }; }).bindgenHook.clang
«derivation /nix/store/c9ym6jk1pf0bx62j29g51k00pp16zf9l-clang-wrapper-16.0.6.drv»
For context, this is what I want to achieve (this is the current solution in the code):
nix-repl> (pkgs.rustPlatform.bindgenHook.override { clang = pkgs.clang_17; }).clang
«derivation /nix/store/cxxk9njrx1zm5ynmsi0wyy9dv58h2zci-clang-wrapper-17.0.6.drv»
(Also, I do want clang 16, but I want to pin it; I'm just using clang 17 in the repl to make sure it is being overridden)
Is there something I'm overlooking?
|
I've added the requested changes, updated to 0.2.1, as well as incorporated some changes from the other PR that were missing from here. I've ran Let me know if any other change in needed, otherwise I'll squash these commits into the other ones. |
diogotcorreia
changed the title
Previously, trying to use `.override {}` on one of the hooks,
specifically `bindgenHook` would yield in an error.
By replacing `callPackage` with `callPackages`, this error is fixed and
the overrides are propagated to the hooks.
Co-Authored-By: Atemu <[email protected]>
diogotcorreia
force-pushed
the
pgvecto.rs
branch
from
108f010 to
0439ada
Compare
|
Ofborg was failing to build EDIT: Seems to still be failing (but a different error now), so I'll wait for someone else's opinion. |
|
@ofborg build postgresqlPackages.pgvecto-rs, postgresqlPackages.pgvecto-rs.passthru.tests The CI failure is unrelated, you can ignore it. |
|
Just deployed this on my Immich server, everything working perfectly! |
diogotcorreia
force-pushed
the
pgvecto.rs
branch
from
0439ada to
8439497
Compare
| }; | ||
|
|
||
| meta = with lib; { | ||
| broken = (stdenv.isLinux && stdenv.isAarch64) || stdenv.isDarwin; |
There was a problem hiding this comment.
Can we add a short comment why this isn't working or set platforms to just x86_64-linux?
There was a problem hiding this comment.
I think @Atemu advocated for broken instead of platforms here: #287632 (comment)
As for why it's broken in linux_aarch64, I personally have no idea as I personally don't have any ARM machine to try it out. Judging from comments in the other PR, it seems to be an issue with bindgen that has already been resolved, but pgvecto-rs hasn't incorporated the changes yet; I'll add that as a comment.
As for darwin, I don't see any official builds from pgvecto-rs, but darwin is listed in the targets section of the rust-toolchain.toml file upstream, so in theory it should work. Again, I don't own any darwin device, so I can't really test this.
There was a problem hiding this comment.
The breakage on aarch64 is essentially this bindgen issue, reported here for pgrx, which is the issue seen in the failed aarch64 build.
There was a problem hiding this comment.
I'll update the comment with the issue link for pgrx since it's more relevant, thanks!
diogotcorreia
force-pushed
the
pgvecto.rs
branch
from
8439497 to
09d893d
Compare
diogotcorreia
force-pushed
the
pgvecto.rs
branch
2 times, most recently
from
c174bce to
8c8a26e
Compare
Co-Authored-By: Daniel Albert <[email protected]> Co-Authored-By: rina <[email protected]>
diogotcorreia
force-pushed
the
pgvecto.rs
branch
from
8c8a26e to
6b97ba6
Compare
|
Just added @esclear as maintainer as well, as requested. Hopefully that's all 🚀 |
|
Thanks! |
Description of changes
Description: Scalable Vector Search in Postgres. Revolutionize Vector Search, not Database.
https://github.com/tensorchord/pgvecto.rs
Used for Immich.
Closes #274509
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/)I have not tested this with Immich before, but as per #274509 (comment), it seems like this version would not work (because of upstream).Keeping this as a draft until Immich moves to a more recent pgvecto.rs version.Edit: see #281192 (comment) (next version of Immich will use pgvecto.rs v0.2.0)
Add a 👍 reaction to pull requests you find important.