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

Jump to bottom

pythonPackages.flatbuffers: init at 1.12.0 #103554

Merged
merged 1 commit into from Nov 18, 2020
Merged

pythonPackages.flatbuffers: init at 1.12.0 #103554

merged 1 commit into from Nov 18, 2020

Conversation

Wulfsta
Copy link
Member

@Wulfsta Wulfsta commented Nov 12, 2020

Motivation for this change

Expose flatbuffers for python. It appears future TensorFlow builds will need this.
@teh
@acowley

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@djanatyn
Copy link
Contributor

I was able to successfully load this package, LGTM:

❯ nix repl
Welcome to Nix version 2.3.8. Type :? for help.

nix-repl> :l
Added 12691 variables.

nix-repl> :b pkgs.python37Packages.flatbuffers
[1 built, 13 copied (70.0 MiB), 12.8 MiB DL]

this derivation produced the following outputs:
  out -> /nix/store/9fmi2pigmakf0dnd2jq2lr3c0zscyiz5-python3.7-flatbuffers-1.12.0

nix-repl> :u pkgs.python38.withPackages(ps: with ps; [ python38Packages.flatbuffers ])
these derivations will be built:
  /nix/store/41wwylbsz2wgnkq1mz3yyjcbprmh7jy5-python3.8-flatbuffers-1.12.0.drv
  /nix/store/kip938br5c83scb34kf8hywh8zr25xyb-python3-3.8.6-env.drv
building '/nix/store/41wwylbsz2wgnkq1mz3yyjcbprmh7jy5-python3.8-flatbuffers-1.12.0.drv'...
...
OK
Finished executing setuptoolsCheckPhase
building '/nix/store/kip938br5c83scb34kf8hywh8zr25xyb-python3-3.8.6-env.drv'...
created 217 symlinks in user environment

[nix-shell:~/repos/nixpkgs]$ python
Python 3.8.6 (default, Sep 23 2020, 13:54:27)
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import flatbuffers
>>> dir(flatbuffers)
['Builder', 'Table', '__builtins__', '__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__path__', '__spec__', 'builder', 'compat', 'compat_range', 'encode', 'number_types', 'packer', 'table', 'util']
>>> exit()

I'm curious if it's possible to pass in flatbuffers (instead of pkgs) without running into infinite recursion issues referencing it? I spent some time exploring what that might look like, but never got a successful build.

@Wulfsta
Copy link
Member Author

Wulfsta commented Nov 12, 2020

@djanatyn The only thing I can think of that might be cleaner is a

let 
  fltbuf = pkgs.flatbuffers;
in
...

but that doesn't change that pkgs has to be passed rather than flatbuffers.

@teh
Copy link
Contributor

teh commented Nov 12, 2020

I think the naming convention is still python3Packages.flatbuffers: init at 1.12.0 even if the source is derived like this. Otherwise LGTM.

@Wulfsta
Copy link
Member Author

Wulfsta commented Nov 13, 2020

Changed commit message and improved the derivation.

IvarWithoutBones
IvarWithoutBones requested changes Nov 13, 2020
View reviewed changes
Copy link
Member

@IvarWithoutBones IvarWithoutBones left a comment

Choose a reason for hiding this comment

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

A few nits :)

pkgs/development/python-modules/flatbuffers/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/flatbuffers/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/flatbuffers/default.nix Show resolved Hide resolved
@Wulfsta Wulfsta force-pushed the flatbuffers-python branch 2 times, most recently from e87061e to fb7ec69 Compare November 14, 2020 03:22
@Wulfsta
Copy link
Member Author

Wulfsta commented Nov 14, 2020 •
edited

Using flatbuffers rather than pkgs.flatbuffers appears to fail. Reverting that change.

@Wulfsta Wulfsta force-pushed the flatbuffers-python branch 2 times, most recently from 580e13e to e07b5cc Compare November 15, 2020 22:13
@jonringer jonringer changed the title flatbuffers: add python api pythonPackages.flatbuffers: init at 1.12.0 Nov 15, 2020
@Wulfsta Wulfsta force-pushed the flatbuffers-python branch 3 times, most recently from 34bd99f to 9f996df Compare November 15, 2020 23:03
jonringer
jonringer reviewed Nov 16, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

tensorflow is now picking this up instead of the flatbuffers at the pkgs package scope. please apply this diff:

diff --git a/pkgs/top-level/python-packages.nix b/pkgs/top-level/python-packages.nix
index d9862a193e4..2432d3335c5 100644
--- a/pkgs/top-level/python-packages.nix
+++ b/pkgs/top-level/python-packages.nix
@@ -7063,6 +7063,7 @@ in {
     nccl = pkgs.nccl_cudatoolkit_10;
     openssl = pkgs.openssl_1_1;
     inherit (pkgs.darwin.apple_sdk.frameworks) Foundation Security;
+    inherit (pkgs) flatbuffers;
   };

   tensorflow-build_2 = callPackage ../development/python-modules/tensorflow/2 {
@@ -7072,6 +7073,7 @@ in {
     nccl = pkgs.nccl_cudatoolkit_11;
     openssl = pkgs.openssl_1_1;
     inherit (pkgs.darwin.apple_sdk.frameworks) Foundation Security;
+    inherit (pkgs) flatbuffers;
   };

   tensorflow-build = self.tensorflow-build_1;

jonringer
jonringer approved these changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM

https://github.com/NixOS/nixpkgs/pull/103554
3 packages built:
python27Packages.flatbuffers python37Packages.flatbuffers python38Packages.flatbuffers

@jonringer jonringer merged commit 3c9c52d into NixOS:master Nov 18, 2020
@Wulfsta Wulfsta mentioned this pull request Nov 27, 2020
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvarWithoutBones IvarWithoutBones IvarWithoutBones requested changes

@jonringer jonringer jonringer approved these changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Wulfsta @djanatyn @teh @jonringer @IvarWithoutBones