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

Add missing Python 3.11 dependencies. #29609

Closed
wants to merge 3 commits into from

Conversation

tvalentyn
Copy link
Contributor

@tvalentyn tvalentyn commented Dec 5, 2023

  • Add two dependencies that were previously excluded for Python 3.11.
  • Modify the scripts to install python-snappy linux headers so that dependencies can be regenerated.

fixes: #25985

Copy link
Contributor

github-actions bot commented Dec 5, 2023

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @damccorm for label python.
R: @Abacn for label build.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@@ -30,6 +30,16 @@
# You will need Python interpreters for all versions supported by Beam, see:
# https://s.apache.org/beam-python-dev-wiki

if ! [[ "$(expr substr $(uname -s) 1 5)" == "Linux" ]]; then
echo This script needs to be executed in a Linux environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: update wiki page since devs mostly update it on their own systems (which is Mac).

Copy link
Contributor Author

@tvalentyn tvalentyn Dec 5, 2023

Choose a reason for hiding this comment

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

I believe the script already fails on Mac due to google-cloud-profiler or some other dependency not installable on Mac, that is why I added this. Can current version of the script (before this PR) run on Mac?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe Wiki already recommends a linux machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe Wiki already recommends a linux machine.

looks like not. I definitely remember script failing on MacOS before.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've run this successfully from my (non-M1) mac before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, added instructions for MacOS. @AnandInguva please help verify that is sufficient. You can check out the pr locally by smth like:

git fetch origin pull/29609/head:pr-29609                                           
git checkout pr-29609    

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need anything else from me here? asking in case if I missed something. As of now, this is not running on M1 Mac.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could try to see what it takes to make it succeed on a Mac but don't spend too much time if you can't get it to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. I will take a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

i tried few things from stackoverflow and python-snappy github page but no luck so far.

@@ -38,6 +38,8 @@ jobs:
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.params.py_ver }}
- name: Install headers required by some python packages
Copy link
Contributor

@AnandInguva AnandInguva Dec 5, 2023

Choose a reason for hiding this comment

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

we might need to install this in the tox tests? for tests that might be using snappy? I don't think we install it tox anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is a gap in beam test coverage. this codepath might be covered by google-internal tests that use TFRecordIo.

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c9c89fe) 37.88% compared to head (e6627d2) 37.88%.
Report is 150 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #29609   +/-   ##
=======================================
  Coverage   37.88%   37.88%           
=======================================
  Files         690      690           
  Lines      101253   101263   +10     
=======================================
+ Hits        38358    38364    +6     
- Misses      61300    61303    +3     
- Partials     1595     1596    +1     
Flag Coverage Δ
go 53.39% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

elif [[ "$(expr substr $(uname -s) 1 6)" == "Darwin" ]]; then
if ! brew list snappy >/dev/null ; then
echo You must install snappy to run this script. Run:
echo brew install snappy
Copy link
Contributor

Choose a reason for hiding this comment

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

Generating requirements from MacOS M1/M2 will remove snappy from the base_image_requirements.txt right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not. MacOS users would have to install snappy package and rerun the script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you could try running the script on macos.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let me try running it on M1 Mac.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, please let me know if you can get it to work. I briefly asked another coworker, seems like it's not enough - maybe restarting shell is required?

We also tried setting CPPFLAGS="-I/usr/local/include -L/usr/local/lib" pip install python-snappy

Copy link
Contributor

@AnandInguva AnandInguva Dec 7, 2023

Choose a reason for hiding this comment

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

I ran > Task :sdks:python:container:py311:generatePythonRequirements FAILED
It failed for me.

  Building wheel for python-snappy (setup.py): started
  Building wheel for python-snappy (setup.py): finished with status 'error'
  error: subprocess-exited-with-error
  
  × python setup.py bdist_wheel did not run successfully.
  │ exit code: 1
  ╰─> [25 lines of output]
      running bdist_wheel
      running build
      running build_py
      creating build
  Running setup.py clean for python-snappy
      creating build/lib.macosx-14.1-arm64-cpython-311
      creating build/lib.macosx-14.1-arm64-cpython-311/snappy
      copying src/snappy/snappy.py -> build/lib.macosx-14.1-arm64-cpython-311/snappy
      copying src/snappy/snappy_cffi.py -> build/lib.macosx-14.1-arm64-cpython-311/snappy
      copying src/snappy/__init__.py -> build/lib.macosx-14.1-arm64-cpython-311/snappy
      copying src/snappy/hadoop_snappy.py -> build/lib.macosx-14.1-arm64-cpython-311/snappy
      copying src/snappy/snappy_formats.py -> build/lib.macosx-14.1-arm64-cpython-311/snappy
      copying src/snappy/__main__.py -> build/lib.macosx-14.1-arm64-cpython-311/snappy
      copying src/snappy/snappy_cffi_builder.py -> build/lib.macosx-14.1-arm64-cpython-311/snappy
      running build_ext
      building 'snappy._snappy' extension
      creating build/temp.macosx-14.1-arm64-cpython-311
      creating build/temp.macosx-14.1-arm64-cpython-311/src
      creating build/temp.macosx-14.1-arm64-cpython-311/src/snappy
      clang -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -I/Users/anandinguva/Desktop/projects/beam/build/python311_requirements_gen/include -I/Users/anandinguva/.pyenv/versions/3.11.5/include/python3.11 -c src/snappy/crc32c.c -o build/temp.macosx-14.1-arm64-cpython-311/src/snappy/crc32c.o
      clang -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -I/Users/anandinguva/Desktop/projects/beam/build/python311_requirements_gen/include -I/Users/anandinguva/.pyenv/versions/3.11.5/include/python3.11 -c src/snappy/snappymodule.cc -o build/temp.macosx-14.1-arm64-cpython-311/src/snappy/snappymodule.o
      src/snappy/snappymodule.cc:33:10: fatal error: 'snappy-c.h' file not found
      #include <snappy-c.h>
               ^~~~~~~~~~~~
      1 error generated.
      error: command '/usr/bin/clang' failed with exit code 1
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for python-snappy
Successfully built bs4 future google-cloud-profiler
Failed to build python-snappy
ERROR: Could not build wheels for python-snappy, which is required to install pyproject.toml-based projects


@tvalentyn
Copy link
Contributor Author

let's decouple snappy upgrade from googlecloudprofiler, sending: #29651

@tvalentyn tvalentyn marked this pull request as draft December 7, 2023 18:54
Copy link
Contributor

Reminder, please take a look at this pr: @damccorm @Abacn

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.

[Task]: Include python-snappy in Python 3.11 containers once python-snappy supports Python 3.11
3 participants