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.

Sign up for GitHub

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

python37Packages.tensorflow: 1.15.2 Fix #96273

Closed
wants to merge 4 commits into from
Closed

python37Packages.tensorflow: 1.15.2 Fix #96273

wants to merge 4 commits into from

Conversation

Rakesh4G
Copy link
Contributor

@Rakesh4G Rakesh4G commented Aug 25, 2020 •
edited

Motivation for this change

Fixing Broken python37Packages.tensorflow: 1.15.2 since
377324c

As per tensorflow/tensorflow#41061 (comment)

Numpy < 1.19 should fix the issue

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.

@Rakesh4G Rakesh4G marked this pull request as ready for review August 25, 2020 12:59
@danieldk
Copy link
Contributor

danieldk commented Aug 25, 2020 •
edited

Maybe we should consider removing Tensorflow 1? It seems like even keeping Tensorflow 2 working is quite a burden.

danieldk
danieldk suggested changes Aug 25, 2020
View reviewed changes
pkgs/top-level/python-packages.nix Outdated
@@ -5095,6 +5095,8 @@ in {
else
callPackage ../development/python-modules/numpy { };

numpy_1_18_5 = callPackage ../development/python-modules/numpy/1.18.5.nix { }
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing semicolon breaks the build.

Suggested change
numpy_1_18_5 = callPackage ../development/python-modules/numpy/1.18.5.nix { }
numpy_1_18_5 = callPackage ../development/python-modules/numpy/1.18.5.nix { };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Rakesh4G Rakesh4G requested a review from danieldk August 25, 2020 13:06
FRidh
FRidh requested changes Aug 25, 2020
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Having another version of numpy is not an option.

In my opinion this expression needs to be removed. If you or anyone else needs older versions around, then you need to maintain them yourselves elsewhere.

@jonringer
Copy link
Contributor

jonringer commented Aug 25, 2020 •
edited

If you or anyone else needs older versions around, then you need to maintain them yourselves elsewhere.

That will likely need to be the path we go.

One option off the top of my head would be to have a top_level attr, then apply an overlay so that there's valid python package sets for those packages:

  tensorflow_1.python37Packages.tensorflow
  tensorflow_2.python37Packages.tensorflow

however, this will likely lead to confusion as picking packages between tensorflow_1.python37Packages and pkgs.python37Packages will likely lead to runtime errors. Also, the python package set in it's current form doesn't allow for many overlays to be composed, so users wouldn't be able to apply their own overlays

@CMCDragonkai
Copy link
Member

Can you leave the breaking of compatibility (i.e. removing TF1) after 20.09 is released.

@danieldk
Copy link
Contributor

danieldk commented Aug 26, 2020 •
edited

Can you leave the breaking of compatibility (i.e. removing TF1) after 20.09 is released.

The build has been broken for ~1.5 month, so I am not sure what value providing the Tensorflow 1 derivation as-is has (I propose to mark it broken now, see #96288 ). Also NixOS 20.03 also has Tensorflow 1.15.2 and it builds succesfully:

https://hydra.nixos.org/build/125031248

So people who need it, but do not want to maintain their own Tensorflow 1 derivation can always compose a Python environment from release-20.03.

@Rakesh4G
Copy link
Contributor Author

Having another version of numpy is not an option.

In my opinion this expression needs to be removed. If you or anyone else needs older versions around, then you need to maintain them yourselves elsewhere.

@FRidh

I have tried to move numpy override inside tensorflow itself. I hope this will resolve this concern.
@CMCDragonkai

@ofborg ofborg bot requested a review from abbradar August 28, 2020 10:57
@danieldk
Copy link
Contributor

danieldk commented Aug 28, 2020 •
edited

I have tried to move numpy override inside tensorflow itself. I hope this will resolve this concern.

The issue is that if somebody uses the tensorflow and numpy derivations, that two versions of numpy end up in Python's path. Python does not have versioned imports.

@FRidh
Copy link
Member

FRidh commented Aug 28, 2020

Like @danieldk said. This is simply not going to work and we need to remove 1.15.

@FRidh FRidh closed this Aug 28, 2020
@Rakesh4G
Copy link
Contributor Author

ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh requested changes

@jonringer jonringer Awaiting requested review from jonringer

@danieldk danieldk Awaiting requested review from danieldk

@abbradar abbradar Awaiting requested review from abbradar

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Rakesh4G @danieldk @jonringer @CMCDragonkai @FRidh