python.pkgs.{tensorflow,tensorflow-estimator,tensorflow-tensorboard}-2: Init at 2.0.0 by Ericson2314 · Pull Request #83518 · NixOS/nixpkgs

Ericson2314 · 2020-03-27T18:06:29Z

Motivation for this change

Major breaking change from 1.x, so treating keeping both versions for now.

Things done

matthewbauer · 2020-03-27T18:30:00Z

pkgs/development/python-modules/tensorflow/2/default.nix

Might be better to use the builtin configurePhase, and just set the flags (dontAddPrefix, ...) to disable the incorrect arguments

matthewbauer · 2020-03-27T18:32:02Z

pkgs/development/python-modules/tensorflow/2/default.nix

This means you want to extend RPATH, not link directly here. I think -rpath ${cuda}/lib -rpath ${cudatoolkit}/lib should accomplish this.

(This may be something we need in a setup hook for cuda)

matthewbauer · 2020-03-27T18:34:09Z

pkgs/top-level/python-packages.nix

maybe there is some reason for packaging every single cudatoolkit version, but perhaps we should just rely on the top level one here, instead of a specific version

I guess this was already here though, so may be okay to leave as is.

pkgs/top-level/python-packages.nix

matthewbauer · 2020-03-27T18:38:13Z

pkgs/development/python-modules/tensorflow/2/default.nix

If possible, I would much prefer a patch that we can send upstream

matthewbauer · 2020-03-27T18:40:04Z

pkgs/development/python-modules/tensorflow/2/default.nix

for this as well

FRidh

I don't think the versioned packages aside from tensorflow should be exposed. Instead, offer a single attribute for each package (tensorflow-estimator, tensorflow-tensorboard) that will depend on tensorflow attribute. Using a different version of tensorflow (and dependents) is a matter of overriding the package set to select the tensorflow version.

pkgs/top-level/python-packages.nix

pkgs/development/python-modules/tensorflow-estimator/2/default.nix

pkgs/development/python-modules/tensorflow-tensorboard/2/default.nix

pkgs/top-level/python-packages.nix

Ericson2314 · 2020-03-27T20:58:38Z

Ah so the defaults between 1 and 2 are already inconsistent between tensorflow and tensorflow-evaluator. I think that means the automatic-default version thing to do will be hard. I rather do it later with a round of breaking changes, if that is alright. It is also complicated by some tensorflow-* using tensorflow, and others being used by tensorflow.

I did fix the unneeded 3rd version, however.

jonringer · 2020-03-27T21:08:05Z

I think tensorflow 1 build was broken for a good while, so some things were let in that probably shouldn't have.

Ericson2314 · 2020-03-27T21:11:39Z

OK I'll just change it then

cdepillabout · 2020-03-28T00:09:27Z

I'm not super familiar with the tensorflow universe, but is there a reason why this is packaging tensorflow-2.0 instead of 2.1?

(cherry picked from commit c186f5e)

TF 1.15 still needs an older version of the tensorflow-estimator package. (cherry picked from commit c539f93)

FRidh · 2020-03-28T06:39:43Z

I'm not super familiar with the tensorflow universe, but is there a reason why this is packaging tensorflow-2.0 instead of 2.1?

That's a very good question, because we typically do not keep older versions. 1.15 was kept as an exception.

timokau · 2020-03-28T11:42:20Z

I think its fine to get this merged first, then update to 2.1.0. This is a great improvement either way. If we decide to go straight to 2.1.0, I'd still like to preserve the 2.0.0 commit in history in case someone needs that particular version or we need to bisect an issue later.

Ericson2314 · 2020-03-28T14:35:24Z

I am working locally to switch to 2.1, since master is already partly (!!) ungraded to 2.1.

If we decide to go straight to 2.1.0, I'd still like to preserve the 2.0.0 commit in history in case someone needs that particular version or we need to bisect an issue later.

That was my thought too, before I had realized it was too late for the clean transition.

Needed for Tensorflow 2.1

Major breaking change from 1.x, so treating keeping both versions for now. (adapted from 33f11be) (adapted from 9e8dea7)

Ericson2314 · 2020-03-30T17:40:16Z

OK fixed @FRidh's concerns, and Matt's can be done later because they apply equally to the existing packages. The ofborg failure is because of a master issue.

Ericson2314 requested review from FRidh and jonringer as code owners March 27, 2020 18:06

Ericson2314 force-pushed the add-tensorflow-2 branch from 16bb15f to 94399fb Compare March 27, 2020 18:08

ofborg bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Mar 27, 2020

matthewbauer reviewed Mar 27, 2020

View reviewed changes

pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved

matthewbauer reviewed Mar 27, 2020

View reviewed changes

pkgs/development/python-modules/tensorflow/2/default.nix Outdated

Copy link

Member

matthewbauer Mar 27, 2020

Choose a reason for hiding this comment

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

for this as well

FRidh suggested changes Mar 27, 2020

View reviewed changes

Ericson2314 force-pushed the add-tensorflow-2 branch from 94399fb to 32e6f54 Compare March 27, 2020 20:54

cdepillabout and others added 2 commits March 28, 2020 03:05

python3Packages.tensorflow-estimator: 1.15.1 -> 2.1.0

9939bd6

(cherry picked from commit c186f5e)

pythonPackages.tensorflow-estimator_1_15_1: init at 1.15.1

6c7b2ac

TF 1.15 still needs an older version of the tensorflow-estimator package. (cherry picked from commit c539f93)

Ericson2314 and others added 3 commits March 30, 2020 04:30

bazel_0_29: init at 0.29.1

d4de62d

Needed for Tensorflow 2.1

python.pkgs.tensorflow{,-bin,-estimator,-tensorboard}-2: Init at 2.1.0

a730888

Major breaking change from 1.x, so treating keeping both versions for now. (adapted from 33f11be) (adapted from 9e8dea7)

Merge remote-tracking branch 'upstream/master' into add-tensorflow-2

1fa3105

Ericson2314 force-pushed the add-tensorflow-2 branch from 7f3b47a to 1fa3105 Compare March 30, 2020 15:54

Ericson2314 requested a review from Profpatsch as a code owner March 30, 2020 15:54

Ericson2314 merged commit 35ac8ba into NixOS:master Mar 30, 2020

uri-canva mentioned this pull request Jan 12, 2023

[19.03] tensorflow-tensorboard requires tensorflow python module to be importable #61173

Closed

Uh oh!

Conversation

Ericson2314 commented Mar 27, 2020

Motivation for this change

Things done

Uh oh!

matthewbauer Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

matthewbauer Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

matthewbauer Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

matthewbauer Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

matthewbauer Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthewbauer Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

matthewbauer Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

FRidh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ericson2314 commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonringer commented Mar 27, 2020

Uh oh!

Ericson2314 commented Mar 27, 2020

Uh oh!

cdepillabout commented Mar 28, 2020

Uh oh!

FRidh commented Mar 28, 2020

Uh oh!

timokau commented Mar 28, 2020

Uh oh!

Ericson2314 commented Mar 28, 2020

Uh oh!

Ericson2314 commented Mar 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Ericson2314 commented Mar 27, 2020 •

edited

Loading