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

Adding tensorflow-io-gcs-filesystem to the build recipe #206

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ganand1
Copy link

@ganand1 ganand1 commented Feb 7, 2022

Checklist

  • [ x] Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • [ x] 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.

@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.

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

For recipe:

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

recipe/meta.yaml Outdated
Comment on lines 277 to 310
- name: tensorflow-io-gcs-filesystem
script: build_io_gcs_fs.sh # [not win]
build:
string: cuda{{ cuda_compiler_version | replace('.', '') }}py{{ CONDA_PY }}h{{ PKG_HASH }}_{{ PKG_BUILDNUM }} # [cuda_compiler_version != "None"]
string: cpu_py{{ CONDA_PY }}h{{ PKG_HASH }}_{{ PKG_BUILDNUM }} # [cuda_compiler_version == "None"]
requirements:
build:
- {{ compiler('c') }}
- {{ compiler('cxx') }}
- {{ compiler('cuda') }} # [cuda_compiler_version != "None"]
- bazel
- bazel >=4.2.1 # [osx and arm64]
host:
- python
- pip
- setuptools
- wheel
- openssl
- {{ pin_subpackage('tensorflow-base', exact=True) }}
run:
- python
- {{ pin_subpackage('tensorflow-base', exact=True) }}
test:
requires:
- pip
commands:
- pip check

Copy link
Contributor

@ngam ngam Feb 7, 2022

Choose a reason for hiding this comment

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

Suggested change
- name: tensorflow-io-gcs-filesystem
script: build_io_gcs_fs.sh # [not win]
build:
string: cuda{{ cuda_compiler_version | replace('.', '') }}py{{ CONDA_PY }}h{{ PKG_HASH }}_{{ PKG_BUILDNUM }} # [cuda_compiler_version != "None"]
string: cpu_py{{ CONDA_PY }}h{{ PKG_HASH }}_{{ PKG_BUILDNUM }} # [cuda_compiler_version == "None"]
requirements:
build:
- {{ compiler('c') }}
- {{ compiler('cxx') }}
- {{ compiler('cuda') }} # [cuda_compiler_version != "None"]
- bazel
- bazel >=4.2.1 # [osx and arm64]
host:
- python
- pip
- setuptools
- wheel
- openssl
- {{ pin_subpackage('tensorflow-base', exact=True) }}
run:
- python
- {{ pin_subpackage('tensorflow-base', exact=True) }}
test:
requires:
- pip
commands:
- pip check
- name: tensorflow-io
script: build_io_gcs_fs.sh # [not win]
build:
string: cuda{{ cuda_compiler_version | replace('.', '') }}py{{ CONDA_PY }}h{{ PKG_HASH }}_{{ PKG_BUILDNUM }} # [cuda_compiler_version != "None"]
string: cpu_py{{ CONDA_PY }}h{{ PKG_HASH }}_{{ PKG_BUILDNUM }} # [cuda_compiler_version == "None"]
requirements:
build:
- {{ compiler('c') }}
- {{ compiler('cxx') }}
- {{ compiler('cuda') }} # [cuda_compiler_version != "None"]
- bazel
- bazel >=4.2.1 # [osx and arm64]
host:
- python
- pip
- setuptools
- wheel
- openssl
- {{ pin_subpackage('tensorflow', exact=True) }}
run:
- python
- {{ pin_subpackage('tensorflow', exact=True) }}
test:
requires:
- pip
commands:
- pip check

Hi @ganand1, looks you're very close. Could you try building tensorflow-io instead of just the gcs-filesystem? tensorflow-io includes tensorflow-io-gcs-filesystem. Also, I would pin against tensorflow, not the base here. This may help, I can give it a shot locally if you want me to (it will compile relatively faster). Let me know if you have any thoughts or observations so far :) thanks for picking this up! We could maybe implement it in a 2.7.1 release or a rebuild of 2.8.0 once everyone is satisfied

recipe/meta.yaml Outdated
folder: tensorflow-estimator
- url: https://github.com/tensorflow/io/archive/refs/tags/v{{ io_gcs_version }}.tar.gz
sha256: 7a4b57f6f438402bab4919c360e931c32f4d8d8afa23116d32a48461dddd91dc
folder: tensorflow-io-gcs-filesystem
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
folder: tensorflow-io-gcs-filesystem
folder: tensorflow-io


set -exuo pipefail

pushd tensorflow-io-gcs-filesystem
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pushd tensorflow-io-gcs-filesystem
pushd tensorflow-io


./configure.sh

bazel build //tensorflow_io_gcs_filesystem/...
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bazel build //tensorflow_io_gcs_filesystem/...
bazel build //tensorflow_io/...

@@ -219,6 +218,7 @@ outputs:
- python
- {{ pin_subpackage('tensorflow-base', exact=True) }}
- {{ pin_subpackage('tensorflow-estimator', exact=True) }}
- {{ pin_subpackage('tensorflow-io-gcs-filesystem', exact=True) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- {{ pin_subpackage('tensorflow-io-gcs-filesystem', exact=True) }}
- {{ pin_subpackage('tensorflow-io', exact=True) }}

We can probably do io only here or we will want to output it tensorflow-io and tensorflow-io-gcs-filesystem separately

@ngam
Copy link
Contributor

ngam commented Feb 7, 2022

@ganand1 please change the title to indicate you're adding io-gcs-filesystem :)

@ganand1 ganand1 force-pushed the update_2.8.0 branch 2 times, most recently from 3f85ad0 to dfe3553 Compare February 7, 2022 22:23
@ganand1
Copy link
Author

ganand1 commented Feb 7, 2022

@ngam - i think we could generate only tensorflow-io-gcs-filesystem. i followed this readme
https://github.com/tensorflow/io/blob/master/docs/development.md'

specifically python setup.py bdist_wheel --data bazel-bin --project tensorflow-io-gcs-filesystem
-- builds the exact wheel we need. i see some versioning differences. have fixed for the same. Lets see if the fix helps with versioning tensorflow-io-gcs-filesystem to 0.24.

@ngam
Copy link
Contributor

ngam commented Feb 7, 2022

Okay, but please at least have the io package depend on tensorflow-base AND estimator (or simply tensorflow). Otherwise, it will fail again. Have you seen the errors from previous runs? They were, "couldn't find tensorflow-estimator" and couldn't find "tensorflow-io-gcs-filesystem needed for tensorflow-io"

@ganand1
Copy link
Author

ganand1 commented Feb 7, 2022

Okay, but please at least have the io package depend on tensorflow-base AND estimator (or simply tensorflow). Otherwise, it will fail again. Have you seen the errors from previous runs? They were, "couldn't find tensorflow-estimator" and couldn't find "tensorflow-io-gcs-filesystem needed for tensorflow-io"

ok changing to tensorflow.

@ngam
Copy link
Contributor

ngam commented Feb 13, 2022

So... @xhochy knows this best than anyone around here I think. He earlier said this:

Yes, that is a very old vendored setuptools version that is Python 3.10 incompatible.

I am looking at your bash script, and I am thinking, maybe you should follow what xhochy did for estimator? Could you try that please?

#!/bin/bash
set -exuo pipefail
pushd tensorflow-estimator
WHEEL_DIR=${PWD}/wheel_dir
mkdir -p ${WHEEL_DIR}
bazel build tensorflow_estimator/tools/pip_package:build_pip_package
bazel-bin/tensorflow_estimator/tools/pip_package/build_pip_package ${WHEEL_DIR}
${PYTHON} -m pip install --no-deps ${WHEEL_DIR}/*.whl
bazel clean
popd

In particular, I am not sure if you're altering or overriding the custom_toolchain we have here in your script (e.g. --expunge and ./configure.sh).

#!/bin/bash
set -exuo pipefail
pushd tensorflow-io
bazel clean --expunge
bazel shutdown
./configure.sh
bazel build //tensorflow_io_gcs_filesystem/...
python setup.py bdist_wheel --data bazel-bin --project tensorflow-io-gcs-filesystem
${PYTHON} -m pip install --no-deps dist/*.whl
bazel clean
popd

Or maybe the problem is something simpler that I cannot see... I am not so sure. It's surprising that it's working for python <3.10

@ganand1
Copy link
Author

ganand1 commented Feb 13, 2022

So... @xhochy knows this best than anyone around here I think. He earlier said this:

Yes, that is a very old vendored setuptools version that is Python 3.10 incompatible.

I am looking at your bash script, and I am thinking, maybe you should follow what xhochy did for estimator? Could you try that please?

#!/bin/bash
set -exuo pipefail
pushd tensorflow-estimator
WHEEL_DIR=${PWD}/wheel_dir
mkdir -p ${WHEEL_DIR}
bazel build tensorflow_estimator/tools/pip_package:build_pip_package
bazel-bin/tensorflow_estimator/tools/pip_package/build_pip_package ${WHEEL_DIR}
${PYTHON} -m pip install --no-deps ${WHEEL_DIR}/*.whl
bazel clean
popd

In particular, I am not sure if you're altering or overriding the custom_toolchain we have here in your script (e.g. --expunge and ./configure.sh).

#!/bin/bash
set -exuo pipefail
pushd tensorflow-io
bazel clean --expunge
bazel shutdown
./configure.sh
bazel build //tensorflow_io_gcs_filesystem/...
python setup.py bdist_wheel --data bazel-bin --project tensorflow-io-gcs-filesystem
${PYTHON} -m pip install --no-deps dist/*.whl
bazel clean
popd

Or maybe the problem is something simpler that I cannot see... I am not so sure. It's surprising that it's working for python <3.10

you mean following tensorflow-estimator and build using build_pip_package like commands?

@ngam
Copy link
Contributor

ngam commented Feb 13, 2022

you mean following tensorflow-estimator and build using build_pip_package like commands?

yes, or more generally, trying to see how the tensorflow-estimator building was translated from upstream to here and then you could mimic it. Maybe that will help, but maybe not. That's the only thing that comes to mind...

@ganand1
Copy link
Author

ganand1 commented Feb 13, 2022

tensorflow-io doesnt have that option..
but errors comes from here
https://github.com/tensorflow/io/blob/b9c150b39c62c8a8b93b346f527d9b2e3591cf5f/WORKSPACE#L99
this is too old...
i am thinking does this support python 3.10?

@ngam
Copy link
Contributor

ngam commented Feb 13, 2022

tensorflow-io doesnt have that option..
but errors comes from here
https://github.com/tensorflow/io/blob/b9c150b39c62c8a8b93b346f527d9b2e3591cf5f/WORKSPACE#L99
this is too old...
i am thinking does this support python 3.10?

I like your patch :)

@ngam
Copy link
Contributor

ngam commented Feb 13, 2022

fingers crossed 🤞

@ganand1
Copy link
Author

ganand1 commented Feb 13, 2022

tensorflow-io doesnt have that option.. but errors comes from here https://github.com/tensorflow/io/blob/b9c150b39c62c8a8b93b346f527d9b2e3591cf5f/WORKSPACE#L99 this is too old... i am thinking does this support python 3.10?

i think it should be fine to remove the lint dependencies as well if this doesnt help. As i read from the build documents, lint has been added for checking lint errors. we could remove it i guess. if this doesnt work will give that a try

@xhochy
Copy link
Member

xhochy commented Feb 13, 2022

tensorflow-io doesnt have that option..
but errors comes from here
https://github.com/tensorflow/io/blob/b9c150b39c62c8a8b93b346f527d9b2e3591cf5f/WORKSPACE#L99
this is too old...
i am thinking does this support python 3.10?

I like your patch :)

Yes, sensible approach 👍

@ganand1 ganand1 force-pushed the update_2.8.0 branch 2 times, most recently from dbdb72a to ac68305 Compare February 13, 2022 22:50
@ganand1
Copy link
Author

ganand1 commented Feb 14, 2022

tensorflow-io doesnt have that option..
but errors comes from here
https://github.com/tensorflow/io/blob/b9c150b39c62c8a8b93b346f527d9b2e3591cf5f/WORKSPACE#L99
this is too old...
i am thinking does this support python 3.10?

I like your patch :)

Yes, sensible approach 👍

have 3.10 running as well. please take a look

@ganand1
Copy link
Author

ganand1 commented Feb 14, 2022

@conda-forge-admin, please restart ci
trying again to see if python3.9 also could pass by running on a different machine

@ganand1
Copy link
Author

ganand1 commented Feb 14, 2022

passing log for python3.10 from the previous run
https://dev.azure.com/conda-forge/84710dde-1620-425b-80d0-4cf5baca359d/_apis/build/builds/458732/logs/111

@ngam
Copy link
Contributor

ngam commented Feb 14, 2022

passing log for python3.10 from the previous run
https://dev.azure.com/conda-forge/84710dde-1620-425b-80d0-4cf5baca359d/_apis/build/builds/458732/logs/111

Very cool!! Was it just that patch or was there something else needed?

@ngam
Copy link
Contributor

ngam commented Feb 14, 2022

BTW, I am happy to build the osx ones here too. Just give me a signal once you think we are good to go, and then I can find a time to build them soon.

@ganand1
Copy link
Author

ganand1 commented Feb 14, 2022

BTW, I am happy to build the osx ones here too. Just give me a signal once you think we are good to go, and then I can find a time to build them soon.

thank you @ngam . please go ahead .. it was the patch - old version incompatibility. I just retriggered to get python3.9 passing which was failing for. unknown reason

please try OSX at your convenience. thanks for the extended help.

@ngam
Copy link
Contributor

ngam commented May 13, 2022

@ganand1 still interested in this?

ngam added a commit to ngam/tensorflow-feedstock that referenced this pull request May 13, 2022
Co-authored-by: ganand1
@ganand1
Copy link
Author

ganand1 commented May 14, 2022

@ganand1 still interested in this?

it will be good to merge it so we could have tensorflow-io-gcs-filesystem released from conda-forge.

@ngam ngam mentioned this pull request May 14, 2022
5 tasks
@ngam
Copy link
Contributor

ngam commented May 16, 2022

@ganand1 still interested in this?

it will be good to merge it so we could have tensorflow-io-gcs-filesystem released from conda-forge.

It requires some decent amount of work though. I built essentially lightly modified version of this, but it fails in practice. Why? Because the package of interest is actually tensorflow-io, which depends on tensorflow-io-gcs-filesystem. But when I tried to build them both together, they refused to cooperate well enough. So some work is needed. So I suggest we move this to a staged-recipes feedstock. Let me look into this in approx two weeks, and I will tag you in the PR over at conda-forge/staged-recipes. I personally want to see this added to conda-forge asap as well :)

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.

5 participants