Skip to content

Big dockerimage cleanup#28010

Merged
jtattermusch merged 26 commits intogrpc:masterfrom
jtattermusch:big_dockerimage_cleanup
Dec 1, 2021
Merged

Big dockerimage cleanup#28010
jtattermusch merged 26 commits intogrpc:masterfrom
jtattermusch:big_dockerimage_cleanup

Conversation

@jtattermusch
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch commented Nov 11, 2021 •
edited
Loading

  • make it clearer why given pre-requisite is being installed (and get rid of some pre-requisites that are no longer needed).
  • differentiate between python dependency for run_tests.py and for actually building grpc-python (which requires much more stuff to be installed).
  • get rid of python2 from the docker images where possible
  • lots of other cleanup
  • rebuild the images from scratch (using the freshest base image)

This also tries to make sure that when run_tests.py spawns itself in a docker container (which is the behavior of --use_docker flag), python3 is actually used for the inner instances of it.

TODOs:

  • [DONE] need to make sure that this doesn't break uploading of results to bigquery
  • [DONE] check with e.g. the boringssl at head job.

@jtattermusch jtattermusch added release notes: no Indicates if PR should not be in release notes lang/all wrapped languages labels Nov 11, 2021
@jtattermusch jtattermusch marked this pull request as ready for review November 12, 2021 18:36
@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Nov 12, 2021

This is now ready for first round of review.

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Nov 12, 2021

Adhoc distribtests run: https://fusion2.corp.google.com/invocations/cc413454-ddc1-4d94-83a0-7d55965f4f36/targets

@jtattermusch jtattermusch force-pushed the big_dockerimage_cleanup branch 2 times, most recently from 911caba to b23fd84 Compare November 17, 2021 10:47
@veblush
Copy link
Copy Markdown
Contributor

veblush commented Nov 17, 2021

Thank you for the clean-up. It looks good to me!

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Nov 18, 2021 •
edited
Loading

The python macos build issue seems real:
https://source.cloud.google.com/results/invocations/a8bbea1d-2bd6-480a-8895-1228c52d192f/targets/github%2Fgrpc%2Ftoplevel_run_tests_invocations%2Frun_tests_python_macos_opt_asyncio/tests

Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'multiprocessing.resource_tracker'
copying python_build/lib.macosx-10.10-x86_64-3.8/grpc/_cython/cygrpc.cpython-38-darwin.so -> src/python/grpcio/grpc/_cython
/Volumes/BuildData/tmpfs/src/github/grpc/workspace_python_macos_opt_asyncio/py38_asyncio/lib/python3.8/site-packages/Cython/Compiler/Main.py:369: FutureWarning: Cython directive 'language_level' not set, using 2 for now (Py2). This will change in a later release! File: /Volumes/BuildData/tmpfs/src/github/grpc/workspace_python_macos_opt_asyncio/src/python/grpcio/grpc/_cython/cygrpc.pxd
  tree = Parsing.p_module(s, pxd, full_module_name)
warning: src/python/grpcio/grpc/_cython/_cygrpc/iomgr.pxd.pxi:26:2: 'grpc_error_handle' redeclared
Found cython-generated files...
/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/multiprocessing/resource_tracker.py:96: UserWarning: resource_tracker: process died unexpectedly, relaunching.  Some resources might leak.
  warnings.warn('resource_tracker: process died unexpectedly, '
+ /Volumes/BuildData/tmpfs/src/github/grpc/workspace_python_macos_opt_asyncio/py38_asyncio/bin/python3.8 -m pip install --no-deps .
Processing /Volumes/BuildData/tmpfs/src/github/grpc/workspace_python_macos_opt_asyncio
Building wheels for collected packages: grpcio
  Building wheel for grpcio (setup.py): started
  Building wheel for grpcio (setup.py): finished with status 'error'
  ERROR: Command errored out with exit status 1:
   command: /Library/Frameworks/Python.framework/Versions/3.7/bin/python3 -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/private/var/folders/wv/_5q8c8j155g873yylt68nd4c0000gn/T/pip-req-build-1sxnlsez/setup.py'"'"'; __file__='"'"'/private/var/folders/wv/_5q8c8j155g873yylt68nd4c0000gn/T/pip-req-build-1sxnlsez/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /private/var/folders/wv/_5q8c8j155g873yylt68nd4c0000gn/T/pip-wheel-vai7xlav --python-tag cp38
       cwd: /private/var/folders/wv/_5q8c8j155g873yylt68nd4c0000gn/T/pip-req-build-1sxnlsez/

Looks like running python build with python3 tools/run_tests/run_tests.py (as opposed to just python tools/run_tests/run_tests.py) is broken for some reason on macos

@gnossen

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Nov 18, 2021

Failures in adhoc run of boringssl_at_head job: https://fusion2.corp.google.com/invocations/cfa4ea7e-84ea-4552-9018-2e155c90742b/targets

@jtattermusch jtattermusch mentioned this pull request Nov 19, 2021
Merged
@jtattermusch jtattermusch force-pushed the big_dockerimage_cleanup branch from b23fd84 to 0c988e0 Compare November 30, 2021 12:26
@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Nov 30, 2021

@veblush @gnossen @apolcyn can you please approve? (Of course I'll make sure the test are passing before merging - currently there's the macos issue mentioned in #28010 (comment))

@veblush veblush self-requested a review November 30, 2021 16:57
@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Dec 1, 2021

Known failures:
#28242

@jtattermusch jtattermusch merged commit 205d756 into grpc:master Dec 1, 2021
@jtattermusch jtattermusch mentioned this pull request Dec 1, 2021
Merged
@veblush veblush mentioned this pull request Dec 2, 2021
Merged
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Dec 2, 2021
@lidizheng lidizheng mentioned this pull request Feb 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gnossen gnossen gnossen approved these changes

@apolcyn apolcyn apolcyn approved these changes

@nicolasnoble nicolasnoble Awaiting requested review from nicolasnoble

+1 more reviewer

@veblush veblush veblush approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@veblush veblush

Labels

bloat/none imported Specifies if the PR has been imported to the internal repository lang/all wrapped languages perf-change/none release notes: no Indicates if PR should not be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jtattermusch @veblush @gnossen @apolcyn