v2.8.0 with system LLVM #213

xhochy · 2022-03-01T15:35:28Z

Checklist

Used a personal fork of the feedstock to propose changes
Bumped the build number (if the version is unchanged)
Reset the build number to 0 (if the version changed)
Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
Ensured the license file is being packaged.

Closes #155

…nda-forge-pinning 2022.02.06.08.21.47

…nda-forge-pinning 2022.02.08.09.38.24

….8.0

conda-forge-linter · 2022-03-01T15:35:32Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

It looks like the 'tensorflow-base' output doesn't have any tests.

xhochy · 2022-03-01T15:39:53Z

This is a draft to use pre-built binaries for llvm and mlir. This depends on the following PRs:

I created a script called generate_llvm.py that generates the Bazel configuration for LLVM from the one that is part of the LLVM codebase. Sadly my efforts were only partially successful as I had a lot of missing symbols at the end. Thus this include the "hack" that the Support libraries link in all static libraries and hope that the linker does --as-needed linkage correctly. If we want to keep this hack, I can probably trim the Python script a bit more down than what has been needed for the non-hacky version.

@h-vetinari @hmaarrfk @ngam @isuruf You're probably the right audience to look at this and assess whether it would be interesting to push it over the finish line.

xhochy · 2022-03-01T15:40:43Z

Locally on osx-*, this seems build successfully 💚

h-vetinari · 2022-03-01T19:25:12Z

This is a draft to use pre-built binaries for llvm and mlir.

This is amazing. ❤️

I don't yet understand the symbol and bazel config issues, but I don't see why we shouldn't merge the precursor PRs and continue iterating here; though perhaps with the one caveat that if we think this has a reasonable chance of working, then I'd prefer to have a dedicated label llvm_tf (instead of llvm_dev) and builds à la: 14.0.0.tf28 or similar.

hmaarrfk · 2022-03-01T21:57:03Z

Are you able to add the dev packages to a channel and ask Azure to build them? Does this get the builds completed in time remotely?

isuruf · 2022-03-01T22:02:17Z

You can have a look at https://github.com/conda-forge/tensorflow-feedstock/pull/78/files as well.

--as-needed doesn't work with static libraries.

xhochy · 2022-03-02T15:38:18Z

You can have a look at https://github.com/conda-forge/tensorflow-feedstock/pull/78/files as well.

--as-needed doesn't work with static libraries.

Yes, the PR was my initial inspiration but sadly the LLVM Bazel definitions got a lot more complicated and diverged from what is built with CMake.

As --as-needed won't work with static libraries, I'll build my own version of it then 🤣

xhochy · 2022-03-02T15:41:07Z

This is a draft to use pre-built binaries for llvm and mlir.

This is amazing. ❤️

I don't yet understand the symbol and bazel config issues, but I don't see why we shouldn't merge the precursor PRs and continue iterating here; though perhaps with the one caveat that if we think this has a reasonable chance of working, then I'd prefer to have a dedicated label llvm_tf (instead of llvm_dev) and builds à la: 14.0.0.tf28 or similar.

Would be nice to discuss that over at conda-forge/llvmdev-feedstock#148 I would also use these binaries for jaxlib but in the end that is also Tensorflow which actually builds it. Once this is discussed and merged, we can get the CI started here.

xhochy · 2022-03-25T19:22:56Z

FTR: the latest commit took 582,69s user 589,93s system 13% cpu 2:29:32,04 total for conda mambabuild -m .ci_support/osx_arm64_python3.10.____cpython.yaml -c conda-forge/label/llvm_tf -c conda-forge/label/mlir_tf recipe on my M1 using an x86 mambaforge.

xhochy · 2022-04-02T12:13:11Z

@conda-forge-admin please rerender

…nda-forge-pinning 2022.04.02.04.11.01

xhochy · 2022-04-10T07:22:09Z

@conda-forge-admin please rerender

…nda-forge-pinning 2022.04.10.07.10.26

xhochy · 2022-04-11T08:21:50Z

@conda-forge/tensorflow I would like to build this (locally). Could I get a review before I spent the CPU hours?

h-vetinari

Did a slightly-less-than-superficial pass, looking very good. Most of my comments are on the level of nits and not critical.

h-vetinari · 2022-04-11T10:48:11Z

recipe/build.sh

@@ -2,6 +2,26 @@

 set -ex

+if [[ "${target_platform}" != "${build_platform}" ]]; then
+  if [ ! -d "llvm-build" ]; then
+    conda create -y -p llvm-build llvmdev=14.0.0.tf280_55c71c9eac9b mlir=14.0.0.tf280_55c71c9eac9b -c conda-forge/label/llvm_tf -c conda-forge/label/mlir_tf


Should we put the 14.0.0.tf280_55c71c9eac9b into cbc.yaml?

Also, this might benefit from a comment. I can see that in cross-compilation we're installing the conda-forge packages and moving the binaries into our build-prefix - I guess what I'm missing is that conda create [pkgs] will automatically use the build_platform as an arch?

h-vetinari · 2022-04-11T10:49:48Z

recipe/gen-bazel-toolchain.sh

+	-e "s:\${MACOSX_SDK_VERSION}:${MACOSX_SDK_VERSION:-}:" \
+	-e "s:\${MACOSX_DEPLOYMENT_TARGET}:${MACOSX_DEPLOYMENT_TARGET:-}:" \


Indent is off here

I think it is correct. for some reason the tab character isn't rendering correclty.

recipe/gen-bazel-toolchain.sh

h-vetinari · 2022-04-11T10:56:44Z

recipe/generate_llvm.py

+# TODO: Automatically extract this
+llvm_commit = "55c71c9eac9bc7f956a05fa9258fad4f86565450"


Another thing for cbc (next to 14.0.0.tf280_55c71c9eac9b)...?

h-vetinari · 2022-04-11T11:03:16Z

recipe/generate_llvm.py

+# Generate LLVM build defintion
+
+PREFIX = Path(os.environ["PREFIX"])
+SYMBOL = "LLVM"


Maybe I'm overlooking something, but I find this a strange variable name. In the loop above you're really resolving symbols from the nm output, but calling a variable containing the string "LLLVM" a symbol is confusing me.

h-vetinari · 2022-04-11T11:07:26Z

recipe/generate_llvm.py

+# Load the BUILD file and run it with the above defined custom functions
+exec(r.text)


Not a big fan of the distance between creating r and using it, especially given it's very short name.

Can we do upstream_build_bazel = r.text after the r= above and then

Suggested change

# Load the BUILD file and run it with the above defined custom functions

exec(r.text)

# Run the upstream BUILD file with the above defined custom functions

exec(upstream_build_bazel)

here?

h-vetinari · 2022-04-11T11:08:45Z

recipe/generate_llvm.py

+# Load the BUILD file and run it with the above defined custom functions
+exec(r.text)


Same here (even larger distance between definition and use)

recipe/meta.yaml

h-vetinari · 2022-04-11T11:13:13Z

recipe/patches/0002-Switch-to-system-llvm-project.patch

+    #tf_http_archive(
+    #    name = "llvm-project",
+    #    build_file = "//third_party/llvm:BUILD",
+    #    sha256 = "22054926b426c66d8f2bc22071365df6e35f3aacf19ad943bc6167d4cae3bebb",
+    #    strip_prefix = "no_valid_prefix",
+    #    system_build_file = "//third_party/systemlibs:llvm.BUILD",
+    #    urls = tf_mirror_urls("https://github.com/invalid/mirror/source.tar.gz"),
+    #)


This commented out code doesn't seem to have existed previously - any reason we're patching it in?

ngam · 2022-04-19T13:19:48Z

Thanks a lot @xhochy. I am not really knowledgeable enough to review the changes here, but I will happily observe and help build when ready.

@isuruf and @hmaarrfk could you have a quick look?

@hmaarrfk, you've built the previous linux/cuda ones --- I am not sure if you have the time or energy this time around, so I am willing to figure out a solution when the time comes. Just let me know. The idea is: let's not delay building because we the lack compute resources (I will get us something to work; I don't have access to local machines, but I will figure out something if/when needed).

Note: 2.9.0-rc0 has been released a week or so ago, so we could try to get that out asap as well.

Tagging @ganand1 for visibility but also if you feel like you could offer thoughts on this PR.

hmaarrfk · 2022-04-26T01:56:29Z

recipe/conda_build_config.yaml

@@ -3,3 +3,5 @@
 # available on 10.13
 MACOSX_SDK_VERSION:        # [osx and x86_64]
  - "10.13"                # [osx and x86_64]
+channel_sources:
+  - conda-forge/label/mlir_tf,conda-forge/label/llvm_tf,conda-forge


I already forgot what mlir stood for. Could you add a sentence or two as to what these are for.

recipe/meta.yaml

hmaarrfk

This is seriously complicated.

Can we have a summary as to why this is useful? We don't get our builds under the CI limit for the GPU builds.

It doesn't seem like these changes can be upstreamed.

I fear like this is simply not going to be maintainable.

xhochy · 2022-04-26T15:56:12Z

Can we have a summary as to why this is useful? We don't get our builds under the CI limit for the GPU builds.

Getting GPU or OSX builds running in CI would have been something that would have offset the maintenance costs here. Without that I feel I have nothing in my hands to argue why this should be maintained.

recipe/meta.yaml

Co-authored-by: h-vetinari <[email protected]> Co-authored-by: Mark Harfouche <[email protected]>

xhochy · 2022-04-26T16:00:13Z

Without that I feel I have nothing in my hands to argue why this should be maintained.

Still, I won't give up here.

ngam and others added 18 commits February 6, 2022 14:15

bump 2.8.0; remove patches

8395a27

MNT: Re-rendered with conda-build 3.21.8, conda-smithy 3.16.2, and co…

7f295b5

…nda-forge-pinning 2022.02.06.08.21.47

force grpc_cpp to 1.42

52a503b

MNT: Re-rendered with conda-build 3.21.8, conda-smithy 3.16.2, and co…

2ac43a0

…nda-forge-pinning 2022.02.06.08.21.47

Update conda_build_config.yaml

664d955

pin numpy to 1.20/1.21 and rerender

feada08

fixing patches, etc.

b688924

try with no grpc_cpp pin

134e684

MNT: Re-rendered with conda-build 3.21.8, conda-smithy 3.16.2, and co…

d0ecf65

…nda-forge-pinning 2022.02.08.09.38.24

remove unnecessary patches; rebase

03d3eb8

add rebased patch

110beb0

remove comments about grpc_cpp

6a6646c

bring back lief_dev for osx

9a2e5ce

Merge branch 'v2.8.0' of github.com:ngam/tensorflow-feedstock into v2…

6da3531

….8.0

tensorflow doesn't support bazel=5 yet

70c4b9a

Pin abseil-cpp

0f00941

Use gcc=9

f1b8802

Enable libtensorflow* on osx-*

3b9892a

Use system LLVM

fb15de2

Implement custom as-needed

b5177f2

ngam mentioned this pull request Mar 3, 2022

cleaner: bump 2.8.0; rebase patches; new numpy pins #205

Merged

5 tasks

Only link librt on Linux

8d2202c

Another round of LLVM deduplication

3562706

MNT: Re-rendered with conda-build 3.21.8, conda-smithy 3.19.0, and co…

db32e65

…nda-forge-pinning 2022.04.02.04.11.01

MNT: Re-rendered with conda-build 3.21.8, conda-smithy 3.19.0, and co…

e5b27aa

…nda-forge-pinning 2022.04.10.07.10.26

xhochy marked this pull request as ready for review April 11, 2022 08:21

xhochy requested review from FarhanTejani, ghego, h-vetinari, hajapy, jschueller, njzjz, waitingkuo and wolfv as code owners April 11, 2022 08:21

h-vetinari reviewed Apr 11, 2022

View reviewed changes

hmaarrfk reviewed Apr 26, 2022

View reviewed changes

recipe/meta.yaml Show resolved Hide resolved

hmaarrfk reviewed Apr 26, 2022

View reviewed changes

xhochy commented Apr 26, 2022

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

Apply suggestions from code review

f83c7f6

Co-authored-by: h-vetinari <[email protected]> Co-authored-by: Mark Harfouche <[email protected]>

xhochy added 2 commits April 27, 2022 15:25

Only build libtfkernel_all_kernels.so

70006d5

Include CUDA_HOME/targets/x86_64-linux/include

aaf90d6

hmaarrfk marked this pull request as draft May 18, 2022 02:03

xhochy closed this Jan 10, 2024

xhochy deleted the v2.8.0-llvm branch January 10, 2024 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v2.8.0 with system LLVM #213

v2.8.0 with system LLVM #213

xhochy commented Mar 1, 2022 •

edited by h-vetinari

Loading

conda-forge-linter commented Mar 1, 2022

xhochy commented Mar 1, 2022

xhochy commented Mar 1, 2022

h-vetinari commented Mar 1, 2022

hmaarrfk commented Mar 1, 2022

isuruf commented Mar 1, 2022

xhochy commented Mar 2, 2022

xhochy commented Mar 2, 2022

xhochy commented Mar 25, 2022

xhochy commented Apr 2, 2022

xhochy commented Apr 10, 2022

xhochy commented Apr 11, 2022

h-vetinari left a comment

h-vetinari Apr 11, 2022

h-vetinari Apr 11, 2022

hmaarrfk Apr 26, 2022

h-vetinari Apr 11, 2022

h-vetinari Apr 11, 2022

h-vetinari Apr 11, 2022

h-vetinari Apr 11, 2022

h-vetinari Apr 11, 2022

ngam commented Apr 19, 2022

hmaarrfk Apr 26, 2022

hmaarrfk left a comment

xhochy commented Apr 26, 2022

xhochy commented Apr 26, 2022

		-e "s:\${MACOSX_SDK_VERSION}:${MACOSX_SDK_VERSION:-}:" \
		-e "s:\${MACOSX_DEPLOYMENT_TARGET}:${MACOSX_DEPLOYMENT_TARGET:-}:" \

		# TODO: Automatically extract this
		llvm_commit = "55c71c9eac9bc7f956a05fa9258fad4f86565450"

		# Load the BUILD file and run it with the above defined custom functions
		exec(r.text)

v2.8.0 with system LLVM #213

v2.8.0 with system LLVM #213

Conversation

xhochy commented Mar 1, 2022 • edited by h-vetinari Loading

conda-forge-linter commented Mar 1, 2022

xhochy commented Mar 1, 2022

xhochy commented Mar 1, 2022

h-vetinari commented Mar 1, 2022

hmaarrfk commented Mar 1, 2022

isuruf commented Mar 1, 2022

xhochy commented Mar 2, 2022

xhochy commented Mar 2, 2022

xhochy commented Mar 25, 2022

xhochy commented Apr 2, 2022

xhochy commented Apr 10, 2022

xhochy commented Apr 11, 2022

h-vetinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngam commented Apr 19, 2022

Choose a reason for hiding this comment

hmaarrfk left a comment

Choose a reason for hiding this comment

xhochy commented Apr 26, 2022

xhochy commented Apr 26, 2022

xhochy commented Mar 1, 2022 •

edited by h-vetinari

Loading