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

Unbreak Tensorflow Haskell bindings #2 #85433

Merged

Conversation

mikesperber
Copy link
Contributor

@mikesperber mikesperber commented Apr 17, 2020

Motivation for this change

This is the second part of the patches to unbreak the Tensorflow bindings for Haskell. (Nr. 3 will be to remove the "broken" annotation once thing settle down.)

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.


tensorflow-proto = super.tensorflow-proto.override {
inherit proto-lens proto-lens-protobuf-types;
tensorflow-proto = (setSourceRoot "tensorflow-proto" (overrideCabal super.tensorflow-proto (drv: { src = tensorflow-haskell; }))).override {
Copy link
Member

Choose a reason for hiding this comment

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

These lines might be a little simpler if you moved the overrideCabal part into the setSourceRoot function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done.

@@ -0,0 +1,23 @@
diff --git a/src/Data/ProtoLens/Encoding/Parser/Internal.hs b/src/Data/ProtoLens/Encoding/Parser/Internal.hs
Copy link
Member

Choose a reason for hiding this comment

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

Is there anyway you could pull these patches from upstream? Normally we don't like to carry around patches here in nixpkgs if at all possible to get them from upstream.

You could always just use a PR you've opened upstream as well and pull them in with fetchPatch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I agree with you and I tried, but nothing applied cleanly.

Copy link
Member

Choose a reason for hiding this comment

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

Okay thanks. In that case, let's keep it like it currently is.

@cdepillabout
Copy link
Member

@mikesperber How do you recommend that I test this is actually working?

@mikesperber
Copy link
Contributor Author

@cdepillabout nix-build the various packages runs test suites, that's what I do.

- get the current Git head of haskell-tensorflow
- adjust dependencies
@cdepillabout
Copy link
Member

I'm testing this with the following command line:

$ nix-build -A haskellPackages.tensorflow-ops -A haskellPackages.tensorflow-opgen -A haskellPackages.tensorflow-mnist-input-data -A haskellPackages.tensorflow-mnist -A haskellPackages.tensorflow-logging -A haskellPackages.tensorflow-core-ops -A haskellPackages.tensorflow -A haskellPackages.tensorflow-proto --arg config '{allowBroken=true;}'

@cdepillabout
Copy link
Member

Everything built correctly. Thanks!

Looking forward to the PR marking these all as unbroken.

@cdepillabout cdepillabout merged this pull request into NixOS:haskell-updates Apr 17, 2020
@cdepillabout
Copy link
Member

Oh, you also might want to add all the proto-lens libraries to

so that you don't have to keep bumping them in configuration-tensorflow.nix everytime a new release is made.

@mikesperber
Copy link
Contributor Author

@cdepillabout I'm not sure I understand: I did that, and they're listed right there. But since they're old, they need patching and will continue to need patching. (My ultimate goal is to bump the dependencies to the current version, which will take care of the problem, but one thing at a time.)

@cdepillabout
Copy link
Member

@mikesperber Oh, I'm sorry! I completely forgot about that first PR!

Okay, please ignore that message then :-)

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

2 participants