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

tensorflow: 1.15.2 -> 1.15.3 #93177

Closed
wants to merge 1 commit into from
Closed

Conversation

avdv
Copy link
Member

@avdv avdv commented Jul 15, 2020

Motivation for this change

tensorflow 1.15.3 was released.

The first commit fixes the build for opt-einsum (see PR #93107), but I also wanted to check whether tensorflow builds.

Actually, my main motivation for this is that I want to upgrade bazel to 3.4.1, but they removed the --incompatible_disable_deprecated_attr_params command line flag, which is used by dm-sonnet. So I tried to upgrade dm-sonnet, hoping that they would not need this flag anymore. But that did not work because it depends on tensorflow and tensorflow did not build because of the broken test in opt-einsum.

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.

@avdv avdv changed the title Tensorflow 1.15.3 tensorflow: 1.15.2 -> 1.15.3 Jul 15, 2020
@ofborg ofborg bot requested review from teh and abbradar July 15, 2020 13:12
@avdv avdv marked this pull request as ready for review July 15, 2020 15:16
Comment on lines 9 to 10
- with pytest.raises(TypeError):
+ with pytest.raises(ValueError):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel like this is a nixpkgs-specific issue, is there a way to upstream it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have created an issue upstream: dgasmith/opt_einsum#147

@ajs124
Copy link
Member

ajs124 commented Aug 18, 2020

Tensorflow doesn't seem to build for me with python 3.7 or 3.6, did it build for you when you opened this PR?

And einsum was bumped to 3.3.0 in #93907

@jonringer
Copy link
Contributor

seems to fail with:

tensorflow/python/lib/core/bfloat16.cc:607:25: note:   no known conversion for argument 2 from '<unresolved overloaded function type>' to 'PyUFuncGenericFunction' {aka 'void (*)(char**, const long int*, const long int*, void*)'}
tensorflow/python/lib/core/bfloat16.cc:645:36: error: no match for call to '(tensorflow::{anonymous}::Initialize()::<lambda(const char*, PyUFuncGenericFunction, const std::array<int, 3>&)>) (const char [8], <unresolved overloaded function type>, const std>
  645 |                       compare_types)) {

With how often tensorflow_1 breaks. It's largely amazing to me that it ever builds.

@matthewbauer
Copy link
Member

I'm skeptical you can get tensorflow 1.15 to work with anything but bazel 0.26 - see https://github.com/tensorflow/tensorflow/blob/v1.15.3/configure.py#L52-L53.

Is tensorflow 1 still being actively used in the community? I was hoping we could fix tensorflow 2 derivations, then remove the tensorflow 1 stuff.

@jonringer
Copy link
Contributor

looks like the older bazel packages were restored, it should be able to call bazel with the older versions.

@CMCDragonkai
Copy link
Member

We are still using TF 1.x. Please keep it around, I hope to find a revision that works.

@ajs124
Copy link
Member

ajs124 commented Aug 24, 2020

Is tensorflow 1 still being actively used in the community?

There are still projects that haven't migrated, e.g. photoprism/photoprism#222

@avdv
Copy link
Member Author

avdv commented Aug 24, 2020

Tensorflow doesn't seem to build for me with python 3.7 or 3.6, did it build for you when you opened this PR?

I started this PR before leaving for vacation and I just got back. I can't really remember, but I think it did not finish successfully.

I'm skeptical you can get tensorflow 1.15 to work with anything but bazel 0.26 - see tensorflow/tensorflow:configure.py@v1.15.3#L52-L53.

I am not trying to build tensorflow with an updated bazel version, it's just that dm-sonnet is build with bazel 3 and tensorflow needs to be build as a dependency. I don't fully understand why. 🤔

@avdv
Copy link
Member Author

avdv commented Aug 25, 2020

seems to fail with:

tensorflow/python/lib/core/bfloat16.cc:607:25: note:   no known conversion for argument 2 from '<unresolved overloaded function type>' to 'PyUFuncGenericFunction' {aka 'void (*)(char**, const long int*, const long int*, void*)'}
tensorflow/python/lib/core/bfloat16.cc:645:36: error: no match for call to '(tensorflow::{anonymous}::Initialize()::<lambda(const char*, PyUFuncGenericFunction, const std::array<int, 3>&)>) (const char [8], <unresolved overloaded function type>, const std>
  645 |                       compare_types)) {

With how often tensorflow_1 breaks. It's largely amazing to me that it ever builds.

Apparently, this is caused by an ABI change of numpy 1.19.0 - see tensorflow/tensorflow#40688

@avdv
Copy link
Member Author

avdv commented Sep 17, 2020

This PR should be unblocked by #98083

@avdv
Copy link
Member Author

avdv commented Sep 21, 2020

Since #98083 has been merged, this is ready to review.

@avdv
Copy link
Member Author

avdv commented Nov 26, 2020

Tensorflow is already at 1.15.4

@avdv avdv closed this Nov 26, 2020
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

6 participants