[BEAM-4032]Download whl for a known target platform on the container image else download sources #16448

AnandInguva · 2022-01-06T19:48:56Z

If a user uses default container image, the target platform is debian. Here we can download whls instead of sources which would decrease startup time of the workers.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

codecov · 2022-01-06T20:50:39Z

Codecov Report

Merging #16448 (468b1a4) into master (928da54) will increase coverage by 8.99%.
The diff coverage is 59.45%.

@@            Coverage Diff             @@
##           master   #16448      +/-   ##
==========================================
+ Coverage   74.60%   83.59%   +8.99%     
==========================================
  Files         653      452     -201     
  Lines       81691    62093   -19598     
==========================================
- Hits        60945    51907    -9038     
+ Misses      19767    10186    -9581     
+ Partials      979        0     -979

Impacted Files	Coverage Δ
...s/python/apache_beam/runners/portability/stager.py	`85.17% <59.45%> (-3.12%)`	⬇️
...pache_beam/runners/portability/portable_metrics.py	`89.65% <0.00%> (-10.35%)`	⬇️
...ks/python/apache_beam/runners/worker/data_plane.py	`87.50% <0.00%> (-1.71%)`	⬇️
sdks/python/apache_beam/dataframe/partitionings.py	`94.53% <0.00%> (-1.51%)`	⬇️
sdks/python/apache_beam/internal/metrics/metric.py	`90.00% <0.00%> (-1.00%)`	⬇️
...pache_beam/runners/interactive/interactive_beam.py	`75.60% <0.00%> (-0.98%)`	⬇️
...runners/interactive/display/pcoll_visualization.py	`85.85% <0.00%> (-0.51%)`	⬇️
...s/python/apache_beam/io/gcp/bigquery_file_loads.py	`87.70% <0.00%> (-0.03%)`	⬇️
sdks/python/apache_beam/metrics/metric.py	`95.38% <0.00%> (ø)`
sdks/python/apache_beam/runners/common.py	`89.98% <0.00%> (ø)`
... and 217 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 928da54...468b1a4. Read the comment docs.

AnandInguva · 2022-01-06T21:38:25Z

cc: @tvalentyn.

Still verifying if these files match the files that the worker looks for in the requirements cache

sdks/python/apache_beam/runners/portability/stager.py

AnandInguva · 2022-01-10T19:42:00Z

Run Python 3.8 Postcommit

AnandInguva · 2022-01-10T19:42:58Z

R: @tvalentyn
cc: @pabloem

sdks/python/apache_beam/runners/portability/stager.py

tvalentyn · 2022-01-12T21:38:54Z

sdks/python/apache_beam/runners/portability/stager.py

    """Downloads SDK package from PyPI and returns path to local path."""
-    package_name = Stager.get_sdk_package_name()
+    package_name = package_name or Stager.get_sdk_package_name()


Leftover?

Or did you mean to combine _download_pypi_package and _download_pypi_sdk_package into the same method (as in, you would pass package_name = Stager.get_sdk_package_name() into _download_pypi_package to download the SDK)? That would make sense, as it's mostly the same code. We can remove the warning specific to the apache beam SDK package.

Yes, I wanted to combine them as they work in similar way. I made a small change to this. This function would fetch apache beam sdk (source or whl) by passing arguments to the _download_pypi_package. I didn't combine them for now but will make a note of it

tvalentyn · 2022-01-12T21:47:59Z

sdks/python/apache_beam/runners/portability/stager.py

+    ]
+
+    if fetch_binary:
+      cmd_args.pop()  # remove the no binary flag


I would make it part of the condition:

if fetch_binary: # add binary flags: else: # add no-binary flags # run the command.

tvalentyn · 2022-01-12T22:14:32Z

sdks/python/apache_beam/runners/portability/stager.py

+          # downloads the binary distributions
+          (
+              populate_requirements_cache if populate_requirements_cache else
+              Stager._populate_requirements_cache_with_whl)(


I would keep one method: _populate_requirements_cache, and add add a flag such as prefer_bdists=True/False (or fetch_binary to match other flags) to avoid code duplication

tvalentyn · 2022-01-12T22:15:24Z

sdks/python/apache_beam/runners/portability/stager.py

-            Stager._populate_requirements_cache)(
-                setup_options.requirements_file, requirements_cache_path)
+        if setup_options.view_as(WorkerOptions).sdk_container_image is None:
+          # downloads the binary distributions


If user passes a custom image we should advise against passing --requirements_file in a warning.

Added a warning. I will add the link to the website on how to use custom containers with requirements once that website page is ready.

AnandInguva · 2022-01-13T23:39:42Z

Run Python 3.8 Postcommit

tvalentyn

As discussed, let's share a one-pager on dev@ with summary of the problem, possible solutions and trade-offs calling out the behavior change for users, indicate which solution you propose but also leave room for feedback & further input.

tvalentyn · 2022-01-14T23:51:55Z

sdks/python/apache_beam/runners/portability/stager.py

@@ -197,6 +198,9 @@ def create_job_resources(options,  # type: PipelineOptions
    resources = []  # type: List[beam_runner_api_pb2.ArtifactInformation]

    setup_options = options.view_as(SetupOptions)
+    # True when sdk_container_image is apache beam image
+    fetch_binary = (


code might be a little more self-explanatory with:

Suggested change

fetch_binary = (

has_default_container_image = (

then, we can use below: fetch_binary=has_default_container_image when passing into a function.

tvalentyn · 2022-01-15T00:13:19Z

sdks/python/apache_beam/runners/portability/stager.py

+        if not fetch_binary:  # display warning.
+          _LOGGER.warning(
+              'Avoid using requirements.txt when using a '
+              'custom container image.')


Wording suggestion for the message:

When using a custom container image, prefer installing additional PyPI dependencies directly into the image, instead of specifying them via runtime options, such as --requirements_file.

tvalentyn · 2022-01-15T00:16:28Z

sdks/python/apache_beam/runners/portability/stager.py

@@ -221,10 +225,16 @@ def create_job_resources(options,  # type: PipelineOptions
                setup_options.requirements_file, REQUIREMENTS_FILE))
        # Populate cache with packages from the requirement file option and
        # stage the files in the cache.
+        if not fetch_binary:  # display warning.


the inline comment is not necessary, the comment is self-explanatory in this case. Btw, https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/ is a good read. Rule 1 applies here.

sdks/python/apache_beam/runners/portability/stager.py

tvalentyn · 2022-01-15T00:33:26Z

sdks/python/apache_beam/runners/portability/stager.py

@@ -162,7 +163,7 @@ def create_job_resources(options,  # type: PipelineOptions
                           temp_dir,  # type: str
                           build_setup_args=None,  # type: Optional[List[str]]
                           pypi_requirements=None, # type: Optional[List[str]]
-                           populate_requirements_cache=None,  # type: Optional[str]
+                           populate_requirements_cache=None,  # type: Optional[Callable]


Thanks. Let's include argument types. I think it's:

Suggested change

populate_requirements_cache=None, # type: Optional[Callable]

populate_requirements_cache=None, # type: Optional[Callable[[str, str], None]]

tvalentyn · 2022-01-15T00:39:27Z

sdks/python/apache_beam/runners/portability/stager.py

+      abi_tag = 'cp%d%d%s' % (
+          sys.version_info[0], sys.version_info[1], abi_suffix
+      )  # ABI tag to use
+      # install each package individually


Add explanation why installing each package individually is necessary in this case.

tvalentyn · 2022-01-18T17:45:01Z

sdks/python/apache_beam/runners/portability/stager.py

@@ -824,32 +890,17 @@ def _download_pypi_sdk_package(
      raise RuntimeError(


Let's remove the arguments that can be inferred (language_version, impl tag, abi tag, platform) from the function signature

tvalentyn · 2022-01-18T18:33:09Z

sdks/python/apache_beam/runners/portability/stager.py

+      language_version_tag='27',
+      language_implementation_tag='cp',
+      abi_tag='cp27mu',
+      platform_tag='manylinux2014_x86_64',


let's create a helper function that returns target platform, and check pip version in that helper. For pip >= 19.3 let's use manylinux2014_x86_64 wheel, otherwise use manylinux2010_x86_64 wheel and add a TODO: when pypa/pip#10760 is addressed, download the wheel based on glibc version in Beam's Python SDK base image.

tvalentyn · 2022-01-18T18:53:10Z

sdks/python/apache_beam/runners/portability/stager.py

+      language_implementation_tag='cp',
+      abi_tag='cp27mu',
+      platform_tag='manylinux2014_x86_64',
+      package_name=None,


(optional) It may make sense to move this param earlier in the argument list

tvalentyn · 2022-01-18T20:01:13Z

sdks/python/apache_beam/runners/portability/stager.py

-    # because we  stage the SDK separately.
+  def _populate_requirements_cache(requirements_file, # type: str
+                                   cache_dir, # type: str
+                                   fetch_binary=False):


To reduce code complexity, I think we can omit fetch_binary param from this flag. Currently, the only case when fetch_binary=False branch is used is custom container users who pass the requirements file. I think it's ok to change the behavior for these users as well, so that we download sources one-by-one from the requirements.txt file in this case, as opposed to download sources from the entire requirements file

tvalentyn · 2022-01-18T20:08:21Z

sdks/python/apache_beam/runners/portability/stager.py

+      for requirement in requirements_to_install:
+        try:
+          # download the bdist compatible with the debian platform
+          # TODO(anandinguva): the platform tag will get updated. Check PEP 600


From offline discussion:

We can rewrite TODO to make it more actionable (do X why Y is fixed)

_download_pypi_package currently does not do the download, but builds the command line

We can likely use the same method for _download_pypi_package and _download_pypi_sdk_package if the download params are the same.

aaltay · 2022-01-27T02:09:12Z

@AnandInguva - what is the next step on this PR?

AnandInguva · 2022-01-27T05:15:35Z

@aaltay

@AnandInguva - what is the next step on this PR?

We had few offline discussions on this approach and encountered few blockers from PyPI to implement efficient functionality. All the discussion can be found here. A different solution for this would be implemented this week.

I will change this to draft PR to preserve the comments that can be used in another PR with discussed solution.

AnandInguva · 2022-02-04T15:51:58Z

Closing this wrt #16633

AnandInguva force-pushed the BEAM-4032 branch from cb8dfdc to 8817eb8 Compare January 6, 2022 20:32

AnandInguva added 3 commits January 10, 2022 14:33

Download whl if the container image is the default container image.

4aa4922

Rename the unit test

e50d40f

Unit test for downloading whl when default container image is used

84a7241

AnandInguva force-pushed the BEAM-4032 branch from 8817eb8 to 84a7241 Compare January 10, 2022 19:34

AnandInguva commented Jan 10, 2022

View reviewed changes

sdks/python/apache_beam/runners/portability/stager.py Show resolved Hide resolved

AnandInguva marked this pull request as ready for review January 10, 2022 19:37

AnandInguva changed the title ~~[BEAM-4032]Download whl if the container image is the default container image.~~ [BEAM-4032]Download whl for a known target platform on the container image else download sources Jan 10, 2022

tvalentyn requested changes Jan 12, 2022

View reviewed changes

AnandInguva and others added 3 commits January 13, 2022 16:41

Support for whl if the target platform on container image is debian

a884877

put cmd_args in the appropriate position

7f5d721

Fixing lint

468b1a4

tvalentyn reviewed Jan 18, 2022

View reviewed changes

AnandInguva marked this pull request as draft January 27, 2022 05:16

AnandInguva closed this Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-4032]Download whl for a known target platform on the container image else download sources #16448

[BEAM-4032]Download whl for a known target platform on the container image else download sources #16448

AnandInguva commented Jan 6, 2022

codecov bot commented Jan 6, 2022 •

edited

Loading

AnandInguva commented Jan 6, 2022

AnandInguva commented Jan 10, 2022

AnandInguva commented Jan 10, 2022 •

edited

Loading

tvalentyn Jan 12, 2022

AnandInguva Jan 13, 2022

tvalentyn Jan 12, 2022

AnandInguva Jan 13, 2022

tvalentyn Jan 12, 2022

AnandInguva Jan 13, 2022

tvalentyn Jan 12, 2022

AnandInguva Jan 13, 2022

AnandInguva commented Jan 13, 2022

tvalentyn left a comment

tvalentyn Jan 14, 2022

tvalentyn Jan 15, 2022

tvalentyn Jan 15, 2022

tvalentyn Jan 15, 2022

tvalentyn Jan 15, 2022

tvalentyn Jan 18, 2022

tvalentyn Jan 18, 2022

tvalentyn Jan 18, 2022

tvalentyn Jan 18, 2022

tvalentyn Jan 18, 2022 •

edited

Loading

aaltay commented Jan 27, 2022

AnandInguva commented Jan 27, 2022 •

edited

Loading

AnandInguva commented Feb 4, 2022

	populate_requirements_cache=None, # type: Optional[Callable]
	populate_requirements_cache=None, # type: Optional[Callable[[str, str], None]]

		@@ -824,32 +890,17 @@ def _download_pypi_sdk_package(
		raise RuntimeError(

[BEAM-4032]Download whl for a known target platform on the container image else download sources #16448

[BEAM-4032]Download whl for a known target platform on the container image else download sources #16448

Conversation

AnandInguva commented Jan 6, 2022

If a user uses default container image, the target platform is debian. Here we can download whls instead of sources which would decrease startup time of the workers.

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

codecov bot commented Jan 6, 2022 • edited Loading

Codecov Report

AnandInguva commented Jan 6, 2022

AnandInguva commented Jan 10, 2022

AnandInguva commented Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnandInguva commented Jan 13, 2022

tvalentyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvalentyn Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

aaltay commented Jan 27, 2022

AnandInguva commented Jan 27, 2022 • edited Loading

AnandInguva commented Feb 4, 2022

`ValidatesRunner` compliance status (on master branch)

codecov bot commented Jan 6, 2022 •

edited

Loading

AnandInguva commented Jan 10, 2022 •

edited

Loading

tvalentyn Jan 18, 2022 •

edited

Loading

AnandInguva commented Jan 27, 2022 •

edited

Loading