[go: nahoru, domu]
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

tensorflow/workspace: re2 dependency does not use release/master branch #34726

Open
perfinion opened this issue Dec 1, 2019 · 10 comments
Open

tensorflow/workspace: re2 dependency does not use release/master branch #34726

perfinion opened this issue Dec 1, 2019 · 10 comments

Comments

@perfinion
Copy link
Member

tensorflow/workspace.bzl has this for the re2 dep:

    tf_http_archive(
        name = "com_googlesource_code_re2",
        sha256 = "d070e2ffc5476c496a6a872a6f246bfddce8e7797d6ba605a7c8d72866743bf9",
        strip_prefix = "re2-506cfa4bffd060c06ec338ce50ea3468daa6c814",
        system_build_file = clean_dep("//third_party/systemlibs:re2.BUILD"),
        urls = [
            "https://storage.googleapis.com/mirror.tensorflow.org/github.com/google/re2/archive/506cfa4bffd060c06ec338ce50ea3468daa6c814.tar.gz",
            "https://github.com/google/re2/archive/506cfa4bffd060c06ec338ce50ea3468daa6c814.tar.gz",
        ],
    )

That commit is google/re2@506cfa4 which is on some random branch called abseil (https://github.com/google/re2/commits/abseil) and incompatible with master or any released version of re2. This completely breaks building against system libs. On Gentoo we have re2-2019.09.01 packaged (and i cant and wont change the package to use a random branch because other packages (eg chromium) need the real released versions not a random branch).

TF wont build against master, it fails with errors about "StringPiece" vs "string_view". I cant figure out how to link properly to the diff but go here https://github.com/google/re2/compare/abseil#diff-6dc69df7618951357bb5fb674c66aa34 and click "files changed" then "showing 120 changed files" then click re2/re2.h. Its fairly obvious why its failing to build.

What even is the abseil branch? The stats say This branch is 166 commits ahead, 167 commits behind master. So its clearly not just some feature branch that is going to be merged in soon.
TF should be using master, if TF needs some new functionality in deps then please make a new re2 release first? I (grudgingly) understand why for eigen and llvm, but re2 has monthly release cycles

@gunan @angerson @martinwicke
@angerson
Copy link
Contributor
angerson commented Dec 2, 2019 •
edited
Loading

Here's where the dependency was changed: 517ad0e

It's only since 1.15 and is present in 1.15, 2.0, and 2.1rc0.

@martinwicke
Copy link
Member
martinwicke commented Dec 2, 2019 via email

@angerson
Copy link
Contributor
angerson commented Dec 6, 2019

Thanks to some insight from RE2 and Absl folks, I think I can explain more about these. The RE2 abseil branch is patched to use Abseil, or specifically:

The master branch is RE2 sans Abseil. It's RE2 with no dependencies. It's what open source users have been building, packaging, wrapping et cetera since 2010.

The abseil branch is RE2 avec Abseil. It's RE2 with a dependency on Abseil. It's a "soft fork" that uses absl::string_view instead of re2::StringPiece, for example.

Since Abseil is normally used at head (LTS releases for Abseil are only tagged twice per year, and Abseil is currently difficult to package partly because of that), the RE2 abseil branch has followed suit and is not frequently tagged.

For whatever reason, TensorFlow uses this Abseil-supporting RE2 soft-fork. It seems like the paths to support system libraries on Gentoo would be:

  • Switch TF to use non-Absl RE2 (which may be a bad idea, depending on the original reasons for the switch)
  • Wait for Abseil to become package-able, and the results of such (e.g. RE2 may use package-friendly abseil in master, or create a separate Gentoo RE2-with-package-friendly-abseil package). I'm not sure if the Abseil team has announced anything to do with this.

@perfinion
Copy link
Member Author

okay thats what i suspected was happening but thanks for confirming everything. :) I'd really like some more announcements directly from re2 / absl people tho instead of second-hand here. In the mean time I can bundle re2 when packaging TF on gentoo and then when things are packagable i''ll work with the maintainers of re2 on gentoo and change things over.

Another part of using absl is that re2 should stick to using only the LTS branches so that things can be packaged easily on all distros that use it. and this means TF also needs to use the LTS absl branches. historically I've never packaged absl because TF bumped hte absl dep too often and there was no tag to package. Maybe absl should move to quarterly tags or something if TF constantly needs new updates from it. I dont actually know what TF uses from absl so not sure exactly how easily it can switch to the LTS branches.

@gunan
Copy link
Contributor
gunan commented Dec 7, 2019

@sanjoy is it feasible for TF to depend on LTS version of absl?
It means updating it only twice a year.

@sanjoy
Copy link
Contributor
sanjoy commented Dec 8, 2019

@sanjoy is it feasible for TF to depend on LTS version of absl?
It means updating it only twice a year.

I suspect that won't be easy. The google3 build of TensorFlow uses absl at head, and so folks will have problems where TF builds fine internally but breaks in open source (when they use a feature available at HEAD but not in the LTS version, for instance).

@martinwicke
Copy link
Member
martinwicke commented Dec 8, 2019 via email

@sanjoy
Copy link
Contributor
sanjoy commented Dec 8, 2019

Just so I understand: what are the problems with bundling re2 and absl with the TF gentoo package in perpetuity? Is it mainly a resource usage (disk space etc.) concern or are there other issues?

TF could not use features not available on the latest LTS. That's not hard test or enforce.

Can this can be done in a way that blaze test fails when someone uses an absl feature that's not available in the latest LTS? If not, then this will introduce another point of divergence between blaze test and the open source bots which isn't ideal (though not a complete deal breaker).

I agree with your point about the backwards incompatible changes.

@martinwicke
Copy link
Member
martinwicke commented Dec 8, 2019 via email

@perfinion
Copy link
Member Author

Just so I understand: what are the problems with bundling re2 and absl with the TF gentoo package in perpetuity? Is it mainly a resource usage (disk space etc.) concern or are there other issues?

First off, this isn't a Gentoo issue, it will be an issue for any distro that packages TensorFlow (I know arch, nix also do, debian does not yet because of bazel issues) I just happened to notice first.

Some more background about why bundling libraries is bad is here: #20284 , specifically https://wiki.gentoo.org/wiki/Why_not_bundle_dependencies https://fedoraproject.org/wiki/Bundled_Libraries
Its mainly a security issue and QA violation to bundle packages. Some distros are super strict about it (eg debian / redhat) I can do it in Gentoo but its not good.

the specific issue here is that chromium depends on re2-without-absl. and now TF depends on re2-with-absl. so, uh, which one would libre2.so be then? and then chromium and tensorflow become mutually incompatible on any linux distro? If it was some obscure library that only TensorFlow used then it might be fine, but many other packages depend on re2 as well so I cant just pull the rug out from everyone else.

The re2 package installs headers in /usr/include/re2/ and the library is /usr/lib64/libre2.so. And that is the released version of re2 so that is what all other packages use. if re2-with-absl is a thing, it cant be installed in those paths cuz then every other package using re2 wont work. For now I just use the bundled re2 package in the TF build which works okay but its suboptimal. If re2 did a hard break, eg their next 2020 tag switched to absl then we could pin the versions and the package manager would figure things out (or complain if there was a conflict until packages were updated).

The other problem with re2 switching to absl is that in its current form, absl is not really packagable at all. I could make a package for it easy enough, but it I cannot package random git hashes, I can only package tags with actual version numbers. The LTS releases would work, but no one uses those so packaging it is currently pointless. How often would absl have to have releases for things to be fine on them? If absl made tags quarterly would that work? or monthly?

@mihaimaruseac mihaimaruseac self-assigned this Feb 10, 2020
@gunan gunan removed their assignment Sep 28, 2020
@mihaimaruseac mihaimaruseac removed their assignment Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@sanjoy @mihaimaruseac @perfinion @martinwicke @gunan @angerson