Skip to content

Conversation

@asford
Copy link

@asford asford commented Jul 19, 2022

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.

grpcio 1.45 was yanked from pypi, for unclear reasons, however a 1.45.1 or 1.45.2 release was never cut to pypi.
The upstream will not release a 1.45.2, in preference of the 1.46 release series.
grpc/grpc#28906

However, the tensorflow 2.8.1 package on conda forge pins to grpcio=1.45.* and abseil-cpp==20210324.2.
conda-forge/tensorflow-feedstock#243

Current solves in conda-forge still solve to grpcio=1.45.0 and grpc-cpp=1.45.2, which is potentially dangerous.
conda-forge/grpcio-feedstock#67

This PR backports the 1.46.x series updates made by @mariusvniekerk in #166 in order to cut a matching 1.45.2 release of grpc-cpp and grpcio for use with tensorflow. This is a "single abseil" release, pinned to 20210324.2

@conda-forge-linter
Copy link

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.

@asford
Copy link
Author

asford commented Jul 19, 2022

@conda-forge-admin, please rerender

@asford asford changed the title Backport 1.46 build config to 1.45 release Backport 1.46 build config for 1.45.2 release Jul 19, 2022
@asford
Copy link
Author

asford commented Jul 19, 2022

edit: I have resolved this issue via a direct merge of the 1.46.x build configuration.

@mariusvniekerk This build failure appears to be because the PREFIX and BUILD_PREFIX values during the conda_build step are not properly configured during the build env setup, and the build script is not detecting cmake.

This appears to be due to the refactor of the package to use outputs.

Did you see anything of this type when building the 1.46.x series?

@asford
Copy link
Author

asford commented Jul 19, 2022

Appears to be conda/conda-build#3310 repro-ing, but not sure why.

@asford
Copy link
Author

asford commented Jul 19, 2022

@conda-forge-admin, please rerender

@conda-forge-linter
Copy link

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

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

…to 1.45.x

No-op merge, taking 1.46.x build config
@conda-forge-linter
Copy link

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.

@asford
Copy link
Author

asford commented Jul 19, 2022

@xhochy, this is intended to support your tensorflow builds.
@mariusvniekerk, this adapts your build configuration.

Given your experiences would you mind giving this a review?

@hmaarrfk
Copy link
Contributor

I think xhochy got busy, and tensorflow just moved to newer abseil with 2.9.1 do you think we still need this?

@asford
Copy link
Author

asford commented Jul 25, 2022

This would be valuable to unblock consistent grpc solves on tensorflow=2.8.1, which my team currently uses.
This isn't a hard block for us, but would be appreciated so we can test grpcio dynamic linking independently of the tensorflow update.

Co-authored-by: h-vetinari <[email protected]>
@h-vetinari
Copy link
Member

The diff looks pretty drastic for an older version, but then:

  • the last 1.45 builds were a months ago
  • the diff with the main branch is much smaller.

I don't consider myself very well-versed in this recipe though, so I'll defer to @mariusvniekerk

@asford
Copy link
Author

asford commented Jul 25, 2022

For context: git diff upstream/1.46.x

diff --git a/recipe/build-cpp.sh b/recipe/build-cpp.sh
index 7879779..794d6e7 100755
--- a/recipe/build-cpp.sh
+++ b/recipe/build-cpp.sh
@@ -2,7 +2,15 @@
 
 set -ex
 
-export CMAKE_ARGS="${CMAKE_ARGS} -DCMAKE_CXX_STANDARD=17"
+echo CONFIG
+
+# Unconditionally set target platform for older abseil version,
+# when updating abseil relax CXX_STANDARD pin
+if [[ "${target_platform}" == osx-* ]]; then
+  export CMAKE_ARGS="${CMAKE_ARGS} -DCMAKE_CXX_STANDARD=14"
+else
+  export CMAKE_ARGS="${CMAKE_ARGS} -DCMAKE_CXX_STANDARD=17"
+fi
 
 if [[ "$CONDA_BUILD_CROSS_COMPILATION" == 1 ]]; then
   (
diff --git a/recipe/meta.yaml b/recipe/meta.yaml
index 020e3a7..392c1b5 100644
--- a/recipe/meta.yaml
+++ b/recipe/meta.yaml
@@ -1,5 +1,5 @@
 {% set name = "grpc-split" %}
-{% set version = "1.46.3" %}
+{% set version = "1.45.2" %}
 
 package:
   name: {{ name | lower }}
@@ -7,7 +7,7 @@ package:
 
 source:
   url: https://github.com/grpc/grpc/archive/v{{ version }}.tar.gz
-  sha256: d6cbf22cb5007af71b61c6be316a79397469c58c82a942552a62e708bce60964
+  sha256: e18b16f7976aab9a36c14c38180f042bb0fd196b75c9fd6a20a2b5f934876ad6
   patches:
     - force-protoc-executable.patch
     - patches-grpcio/0001-Monkey-patch-distutils.ccompiler.spawn-to-elide-std-.patch
@@ -17,7 +17,7 @@ source:
     - patches-grpcio/0001-fix-win-setup.patch          # [win]
 
 build:
-  number: 2
+  number: 5
 
 outputs:
   - name: grpc-cpp
@@ -35,13 +35,15 @@ outputs:
         - libprotobuf
         - ninja
         # We need all host deps also in build for cross-compiling
-        - abseil-cpp 20211102.0  # [build_platform != target_platform]
+        - abseil-cpp 20210324.2  # [build_platform != target_platform]
         - c-ares      # [build_platform != target_platform]
         - re2         # [build_platform != target_platform]
         - openssl     # [build_platform != target_platform]
         - zlib        # [build_platform != target_platform]
       host:
-        - abseil-cpp 20211102.0
+        # Note, pinned to older abseil version.
+        # Update to build.sh required if moving to updated abseil.
+        - abseil-cpp 20210324.2
         - c-ares
         - libprotobuf
         - re2
@@ -59,7 +61,7 @@ outputs:
         - python                                 # [build_platform != target_platform]
         - cross-python_{{ target_platform }}     # [build_platform != target_platform]
         - cython                                 # [build_platform != target_platform]
-        - abseil-cpp 20211102.0                  # [build_platform != target_platform]
+        - abseil-cpp 20210324.2                  # [build_platform != target_platform]
         - {{ compiler('c') }}
         - {{ compiler('cxx') }}
       host:
@@ -68,7 +70,7 @@ outputs:
         - setuptools
         - cython
         - six >=1.6.0
-        - abseil-cpp 20211102.0
+        - abseil-cpp 20210324.2
         - {{ pin_subpackage('grpc-cpp', exact=True) }}
         - c-ares
         - libprotobuf

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Taking a look at this again now that I'm becoming more familiar with the feedstock again.

It's mergeable in principle, and if you rebase this PR, I'd be happy to review.

Comment on lines +7 to 12
# Unconditionally set target platform for older abseil version,
# when updating abseil relax CXX_STANDARD pin
if [[ "${target_platform}" == osx-* ]]; then
export CMAKE_ARGS="${CMAKE_ARGS} -DCMAKE_CXX_STANDARD=14"
else
export CMAKE_ARGS="${CMAKE_ARGS} -DCMAKE_CXX_STANDARD=17"
Copy link
Member

Choose a reason for hiding this comment

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

That comment makes little sense to me (why "target platform", when the variable being set is CMAKE_CXX_STANDARD?). It's also dodgy to use a different C++ standard compared to what was used to compile abseil. Older abseil builds didn't encode that information, though for >=20211102 you can use the right version for the abseil you're using. However, this currently leads into other issues due to the way grpc declares its dependence on abseil (see #213), so I'm fine to let this pass if it works for you.

@h-vetinari
Copy link
Member

@asford
Conda-forge now uses 1.47 globally (and soon we'll start a migration to 1.49). Is there anything from this PR that you still need? Otherwise I'll close it in a few days.

@h-vetinari
Copy link
Member

Closing as stale

@h-vetinari h-vetinari closed this Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants