-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe Wiki already recommends a linux machine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
looks like not. I definitely remember script failing on MacOS before. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've run this successfully from my (non-M1) mac before There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. I will take a look. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
exit 1 | ||
fi | ||
|
||
if ! dpkg -s libsnappy-dev >/dev/null ; then | ||
echo You must install libsnappy-dev to run this script. Run: | ||
echo sudo apt install libsnappy-dev | ||
fi | ||
|
||
if [[ $# -lt 2 ]]; then | ||
printf "Example usage: \n$> ./sdks/python/container/run_generate_requirements.sh 3.8 <sdk_tarball>" | ||
printf "\n\where 3.8 is the Python major.minor version." | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.