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

postgresqlPackages.pgvecto-rs: init at 0.2.1 #287632

Closed
wants to merge 3 commits into from

Conversation

esclear
Copy link
Contributor

@esclear esclear commented Feb 10, 2024

Description of changes

From the pgvecto.rs repository:

pgvecto.rs is a Postgres extension that provides vector similarity search functions. It is written in Rust and based on pgrx.

Big thanks to @katrinafyi, who helped me getting the clang build going.

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.

@esclear esclear changed the title init pgvectors at 0.1.13 init pgvectors at 0.2.0 Feb 10, 2024
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Feb 10, 2024
@sikmir
Copy link
Member

sikmir commented Feb 10, 2024

Commit message pgvectors: init at 0.2.0, please.

@esclear esclear changed the title init pgvectors at 0.2.0 pgvectors: init at 0.2.0 Feb 10, 2024
@esclear
Copy link
Contributor Author

esclear commented Feb 23, 2024

@thoughtpolice @marsam
Hi! Could one of you, as codeowners for postgresql, maybe review this PR?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3539

@SuperSandro2000
Copy link
Member

@ofborg build postgresqlPackages.pgvectors

@SuperSandro2000
Copy link
Member

Fails to build on aarch64-linux, see ofborg log

@esclear esclear force-pushed the add-pgvecto-rs branch 2 times, most recently from e8e8d5f to 6e94cf0 Compare February 29, 2024 18:47
@marsam
Copy link
Contributor

marsam commented Feb 29, 2024

Hi, sorry for the delay. It looks great, thank you! I'll take a proper look tonight

@esclear
Copy link
Contributor Author

esclear commented Feb 29, 2024

Thanks for checking, Sandro.

This appears to be a known issue with bindgen < 0.69.
pgrx 0.11.3 and upwards pull in bindgen >= 0.69.

pgvecto.rs 0.2.0 depends on pgrx 0.11.2 and thus does not include the fix yet, but will include it in the next release.
Thus I removed aarch64 from the platform list again, and will add it back with the next release of pgvecto.rs.

@esclear
Copy link
Contributor Author

esclear commented Mar 4, 2024

pgvecto.rs v0.2.1 was released today, I updated this PR to reflect that.
Unfortunately this does not use pgrx >= 0.11.3 yet, so aarch64 is still not part of the platform list.

@esclear esclear changed the title pgvectors: init at 0.2.0 pgvectors: init at 0.2.1 Mar 4, 2024
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Mar 5, 2024
@marsam
Copy link
Contributor

marsam commented Mar 5, 2024

Hi, sorry for the delay.
I've made some minor changes, and integrated the NixOS test from the other PR. I hope you don't mind.

@GrahamcOfBorg test pgvecto-rs

@marsam marsam changed the title pgvectors: init at 0.2.1 postgresqlPackages.pgvecto-rs: init at 0.2.1 Mar 5, 2024
};

nativeBuildInputs = [
rustPlatform.bindgenHook
Copy link
Member

Choose a reason for hiding this comment

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

Clang version isn't locked here like the other PR does.

meta = with lib; {
description = "Scalable Vector database plugin for Postgres, written in Rust, specifically designed for LLM";
homepage = "https://github.com/tensorchord/pgvecto.rs";
maintainers = with maintainers; [ esclear ];
Copy link
Member

Choose a reason for hiding this comment

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

@diogotcorreia would you like to be added as a maintainer here?

Copy link
Member

Choose a reason for hiding this comment

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

Fine by me!

description = "Scalable Vector database plugin for Postgres, written in Rust, specifically designed for LLM";
homepage = "https://github.com/tensorchord/pgvecto.rs";
maintainers = with maintainers; [ esclear ];
platforms = [ "x86_64-linux" ];
Copy link
Member

Choose a reason for hiding this comment

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

From the other PR:

Suggested change
platforms = [ "x86_64-linux" ];
broken = (stdenv.isLinux && stdenv.isAarch64) || stdenv.isDarwin;

Copy link
Contributor Author

@esclear esclear Mar 5, 2024

Choose a reason for hiding this comment

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

Doesn't that imply, that it is working / supported on all linux platforms, except for aarch64?
I.e. support on ["armv5tel-linux" "armv6l-linux" "armv7a-linux" "armv7l-linux" "i686-linux" "loongarch64-linux" "m68k-linux" "microblaze-linux" "microblazeel-linux" "mips-linux" "mips64-linux" "mips64el-linux" "mipsel-linux" "powerpc64-linux" "powerpc64le-linux" "riscv32-linux" "riscv64-linux" "s390-linux" "s390x-linux"]?

This does not seem to match the platforms that pgvecto.rs supports.

Copy link
Member

Choose a reason for hiding this comment

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

For platforms, we usually list all the platforms that are not explicitly unsupported or cannot be supported for this package. For example, macOS-specific packages will only have platforms.darwin.
A generic rust package however should theoretically compile on any platform where its dependencies can be compiled on.

The only platforms that are important to list here are the 4 platforms which Nixpkgs properly supports. If those are all supported in theory, just say platforms.all (default). If you know a package is specific for all UNIX platforms or only Linux from the get-go (i.e. util-linux), obviously mark it as such. Those are kinda rare though; even traditionally UNIX-only libraries such as GTK or Xorg theoretically function on Windows or macOS too these days.

If there is nothing preventing a build from working in theory but it doesn't compile or breaks at runtime for some reason, you mark it as broken as I suggested here.

Does pgvecto.rs have any explicit platform constraints? I was not able to find any in their documentation.

@esclear
Copy link
Contributor Author

esclear commented Mar 7, 2024

Closing this in favor of #281192.

@esclear esclear closed this Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants