Add pyproject.toml #28385

AnandInguva · 2023-09-09T18:09:03Z

Context

Directly calling setup.py for installing the Python package is deprecated. python setup.py install will throw a warning:

SetuptoolsDeprecationWarning: setup.py install is deprecated.
Use build and pip and other standards-based tools.

The new replacement for setup.py is PEP 517 and PEP 621.
- PEP 621: The package's metadata is now specified in pyproject.toml. The TOML file is declarative and cannot contain arbitrary code unlike setup.py.

This PR provides

Replace setup.py sdist with build. This will install the build dependencies in an isolated environments and then generates sdist. Setuptools itself strongly suggests using this and to avoid using setup.py sdist since it is treated as deprecated. You can find more at https://github.com/pypa/setuptools/blob/245d72a8aa4d47e1811425213aba2a06a0bb64fa/docs/deprecated/commands.rst#L10
- Modify the CI to build the sdist with python -m build
Remove the build-requirements.txt since we move those dependencies into pyproject.toml.
Remove the code in gen_protos.py where we install build dependencies. This is not required since build-requirements are always installed from pyporject.toml

Other notable changes:

Change several Linux-based unit test suites to install SDK from a wheel. This speeds up suites consisting of multiple isolated builds by reusing the the binary distribution that is built once for entire suite.
grpcio is not importable during distribution creation, therefore call gen_protos.py in a subprocess.
The format of sdist is changed from .zip to .tar.gz since we are going to use build module for building sdist

Fixes: #20051
Fixes: #26266

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

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
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.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

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

AnandInguva · 2023-09-09T18:14:32Z

R : @tvalentyn

codecov · 2023-09-09T18:17:57Z

Codecov Report

Merging #28385 (dad39ec) into master (e130352) will decrease coverage by 33.81%.
Report is 14 commits behind head on master.
The diff coverage is 6.89%.

@@             Coverage Diff             @@
##           master   #28385       +/-   ##
===========================================
- Coverage   72.16%   38.36%   -33.81%     
===========================================
  Files         686      686               
  Lines      101554   101672      +118     
===========================================
- Hits        73291    39009    -34282     
- Misses      26684    61087    +34403     
+ Partials     1579     1576        -3

Flag	Coverage Δ
go	`53.40% <ø> (-0.04%)`	⬇️
python	`29.98% <6.89%> (-52.64%)`	⬇️

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

Files	Coverage Δ
sdks/python/apache_beam/runners/common.py	`68.67% <100.00%> (-20.04%)`	⬇️
...s/python/apache_beam/runners/portability/stager.py	`23.65% <25.00%> (-60.74%)`	⬇️
sdks/python/setup.py	`0.00% <0.00%> (ø)`
sdks/python/gen_protos.py	`0.00% <0.00%> (ø)`

... and 310 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sdks/python/scripts/run_pytest.sh

sdks/python/scripts/run_tox.sh

github-actions · 2023-09-09T20:05:44Z

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

R: @riteshghorse for label python.
R: @damccorm for label build.
R: @bvolpato for label io.

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

AnandInguva · 2023-09-10T06:06:00Z

Run Seed Job

AnandInguva · 2023-09-10T06:16:29Z

retest this please

AnandInguva · 2023-09-10T08:11:09Z

Run Seed Job

AnandInguva · 2023-09-10T08:34:45Z

retest this please

AnandInguva · 2023-09-10T11:49:00Z

sdks/python/test-suites/tox/common.gradle

+        }
+}
+
+getVersionsAsList('python_versions').each { it ->


left over work. remove this

AnandInguva · 2023-09-10T11:51:11Z

Run Seed Job

AnandInguva · 2023-09-10T13:54:16Z

retest this please

AnandInguva · 2023-09-10T16:37:16Z

Run Seed Job

AnandInguva · 2023-09-10T17:25:22Z

retest this please

.github/workflows/beam_PreCommit_Python_Dataframes.yml

.github/workflows/build_wheels.yml

tvalentyn · 2023-09-11T18:32:52Z

.github/workflows/typescript_tests.yml

          pip install 'pandas>=1.0,<1.5'
-          python setup.py develop
+          python install -e .


should this be pip install ?

tvalentyn · 2023-09-11T18:42:29Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

-            project.exec {
-              executable 'sh'
-              args '-c', ". ${project.ext.envdir}/bin/activate && cd ${copiedPyRoot} && scripts/run_tox.sh $tox_env $distTarBall '$posargs'"
+          if (project.hasProperty('useWheelDistribution')) {


why do we need both modes?

Precommits running unittest will have this flag and we will use wheels to install apache beam for precommit tests. For tasks like pylint, docs, mypy, we don't need installation of wheels. Tox will take care of running these suits by installing required deps.

wdyt about automatically adding useWheelDistribution whenever platform is linux so that we don't have to add it in individual test suites?

sdks/python/apache_beam/examples/kafkataxi/README.md

sdks/python/setup.py

tvalentyn · 2023-09-11T20:35:55Z

sdks/python/setup.py

      install_requires=[
+          'build>=0.9.0,<0.11.0',


interesting. is it common to add build as a dependency ? Also why not 1.x ?

Since we use it in stager.py, I added it so that it won't cause a breaking change for the users.

It seems 1.x was released recently. I will change the bounds. Thanks for noticing. I made this commit a long time back :p

i think it should at most be an optional dependency, like snappy

tvalentyn · 2023-09-11T20:36:22Z

sdks/python/test-suites/tox/common.gradle

@@ -42,5 +42,5 @@ project.tasks.register("preCommitPy${pythonVersionSuffix}") {
          dependsOn = ["testPy38Cython"]
      } else {
          dependsOn = ["testPy${pythonVersionSuffix}Cloud", "testPy${pythonVersionSuffix}Cython"]
-      }
+        }


unnecessary change

sdks/python/container/Dockerfile

sdks/python/apache_beam/runners/dataflow/internal/clients/cloudbuild/__init__.py

AnandInguva

I addressed few comments. I need to take a look on the unanswered comments. In the mean time, LMK if you have anymore questions

.github/workflows/beam_PreCommit_Python_Dataframes.yml

.github/workflows/typescript_tests.yml

AnandInguva · 2023-09-11T21:15:08Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

-            project.exec {
-              executable 'sh'
-              args '-c', ". ${project.ext.envdir}/bin/activate && cd ${copiedPyRoot} && scripts/run_tox.sh $tox_env $distTarBall '$posargs'"
+          if (project.hasProperty('useWheelDistribution')) {


Precommits running unittest will have this flag and we will use wheels to install apache beam for precommit tests. For tasks like pylint, docs, mypy, we don't need installation of wheels. Tox will take care of running these suits by installing required deps.

AnandInguva · 2023-09-11T21:15:33Z

sdks/python/apache_beam/coders/slow_coders_test.py

@@ -25,14 +25,17 @@
 from apache_beam.coders.coders_test_common import *


+@unittest.skip(
+    'Add a test for non-compiled implementation.'
+    'https://github.com/apache/beam/issues/28307')


Sg. I will follow up in a different PR

sdks/python/scripts/run_pytest.sh

AnandInguva · 2023-09-11T21:19:17Z

sdks/python/apache_beam/io/gcp/bigquery_test.py

@@ -103,6 +102,51 @@ def _load_or_default(filename):
    return {}


+_DESTINATION_ELEMENT_PAIRS = [


https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html#which-import-mode

_ELEMENTS is not able to imported. I suspect it's because we use --import-mode=importlib in pytest command and in the documentation above, they state that test modules are not importable by each other.

AnandInguva · 2023-09-11T21:20:52Z

sdks/python/setup.py

      install_requires=[
+          'build>=0.9.0,<0.11.0',


Since we use it in stager.py, I added it so that it won't cause a breaking change for the users.

It seems 1.x was released recently. I will change the bounds. Thanks for noticing. I made this commit a long time back :p

sdks/python/test-suites/tox/common.gradle

sdks/python/apache_beam/examples/kafkataxi/README.md

tvalentyn · 2023-09-15T21:36:38Z

buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

-              args '-c', ". ${project.ext.envdir}/bin/activate && cd ${copiedPyRoot} && scripts/run_tox.sh $tox_env $distTarBall '$posargs'"
+          if (project.hasProperty('useWheelDistribution')) {
+            def pythonVersionNumber  = project.ext.pythonVersion.replace('.', '')
+            dependsOn ":sdks:python:bdistPy${pythonVersionNumber}linux"


how do we know the platform for the wheel? we have many different test suites.

All the tests that use this flag run on Linux.

tvalentyn · 2023-09-15T21:39:56Z

sdks/python/container/py310/base_image_requirements.txt

@@ -26,11 +26,12 @@ astunparse==1.6.3
 attrs==23.1.0


i'd revert changes to these for now while you are iterating on the pr.

Update numpy bounds

Update setup.py Remove ImportError from gen_protos.py Update subprocess run and raise RuntimeError if proto generation fails Print output of setup.py Fix linting issues

Modify buildPython task

Upgrade pip in Dockerfile Move _ELEMENTS to shared file. tests are not importable by each other Add missing element Remove shared_test_variables Remove installing wheel in a test suite Retry run_tox.sh with no installPkg flag Remove natural language test. codepath is covered in the postCommits. Add back tox exit code

FIx toxTask name Add no-extra test suite to precommit and remove GH duplicate ubuntu test Skip failing non-cython test Fix tox test name

Fix mypy issue

AnandInguva · 2023-10-11T14:53:57Z

Run Seed Job

AnandInguva · 2023-10-11T15:09:39Z

Run Python_Coverage PreCommit

AnandInguva · 2023-10-11T15:09:46Z

Run Python_PVR_Flink PreCommit

AnandInguva · 2023-10-11T15:10:00Z

Run Java PreCommit

AnandInguva · 2023-10-11T16:22:15Z

https://ci-beam.apache.org/job/beam_PreCommit_Python_Coverage_Phrase/69/ - python coverage test suite passes with expected changes on Jenkins. For GHA, it will be reflected once it is merged.

AnandInguva · 2023-10-12T19:15:01Z

If there are any failures, please comment here and I will try to forward fix it.

This reverts commit a94d29f.

* Add pyproject.toml Update numpy bounds * Use subprocess to run grpcio since it is not imported in pyproject.toml Update setup.py Remove ImportError from gen_protos.py Update subprocess run and raise RuntimeError if proto generation fails Print output of setup.py Fix linting issues * Remove build-requirements.txt and use build to build the sdist Modify buildPython task * Use wheels to run tox precommit tests Upgrade pip in Dockerfile Move _ELEMENTS to shared file. tests are not importable by each other Add missing element Remove shared_test_variables Remove installing wheel in a test suite Retry run_tox.sh with no installPkg flag Remove natural language test. codepath is covered in the postCommits. Add back tox exit code * Remove cython tests. default tests will run with Cython extensions FIx toxTask name Add no-extra test suite to precommit and remove GH duplicate ubuntu test Skip failing non-cython test Fix tox test name * Force type cast inputs to list * Update stager to use build. If it fails, use legacy setup to build sdist Fix mypy issue * Remove cython env and build-requirements for tox.ini

Abacn · 2023-10-17T15:34:32Z

It seems ARM postcommit were failing after this change:

Last passed run https://github.com/apache/beam/actions/runs/6499261265 at 223dded (before it was flaky on some of the py version but at least one py version could succeed)

First failing run https://github.com/apache/beam/actions/runs/6502267792/job/17661039890 at 0586161

there are 4 PRs merged and this is the only python change

the error message is

2023-10-13T04:08:33.8802524Z  WARNING. apache_beam.runners.dataflow.dataflow_runner:dataflow_runner.py:202
2023-10-13T03:48:37.128Z: JOB_MESSAGE_WARNING: A worker was unable to start up.
Error: Unable to pull container image due to error: image pull request failed with error:
Error response from daemon: manifest for us.gcr.io/apache-beam-testing/github-actions/beam_python3.9_sdk:20231013-001134756942144 not found: manifest unknown:
Failed to fetch "20231013-001134756942144" from request "/v2/apache-beam-testing/github-actions/beam_python3.9_sdk/manifests/20231013-001134756942144"..
This is likely due to an invalid SDK container image URL.
Please verify any provided SDK container image is valid and that Dataflow workers have permissions to pull image.

2023-10-13T04:08:33.8845726Z  ERROR apache_beam.runners.dataflow.dataflow_runner:dataflow_runner.py:770
Console URL: https://console.cloud.google.com/dataflow/jobs/us-central1/2023-10-12_20_46_29-13701384909164481751?project=apache-beam-testing

AnandInguva · 2023-10-17T16:27:01Z

From this run - https://github.com/apache/beam/actions/runs/6502267792/job/17661040535, I see one of the tests in the postcommits is failing. It was also seen on Jenkins. I have merged the solution 20 hours back. PTAL #28990. That would solve the failing test. let's monitor the tests and see if they go green for the next runs.

AnandInguva · 2023-10-17T16:33:23Z

I started a run - https://github.com/apache/beam/actions/runs/6550155250 on the master. Will monitor it for any related failures to this PR.

lazarillo · 2023-11-09T11:54:01Z

sdks/python/apache_beam/io/gcp/bigquery_test.py

+    {
+        'name': 'spark', 'language': 'py'
+    },
+    {


Just curious, are lines 117 - 119 here a typo? It seems to be a repeat of 111-113.

beam/sdks/python/apache_beam/io/gcp/bigquery_file_loads_test.py

Line 109 in 5284773

_ELEMENTS = [elm[1] for elm in _DESTINATION_ELEMENT_PAIRS]

it is the same as the above variable. It is defined here because when using pytest with --import-mode=importlib, the test modules are not importable by each other hence we defined the same variable statically here.

github-actions bot added python build website infra examples docker io gcp labels Sep 9, 2023

AnandInguva mentioned this pull request Sep 6, 2023

[Task]: Remove non-cythonized tests, codepath and test suites #28385 #28307

Open

15 tasks

AnandInguva commented Sep 9, 2023

View reviewed changes

sdks/python/scripts/run_pytest.sh Show resolved Hide resolved

AnandInguva commented Sep 9, 2023

View reviewed changes

sdks/python/scripts/run_tox.sh Outdated Show resolved Hide resolved

github-actions bot added the Next Action: Reviewers label Sep 9, 2023

AnandInguva commented Sep 10, 2023

View reviewed changes

tvalentyn reviewed Sep 11, 2023

View reviewed changes

AnandInguva commented Sep 11, 2023

View reviewed changes

tvalentyn reviewed Sep 15, 2023

View reviewed changes

github-actions bot added java go labels Sep 21, 2023

AnandInguva force-pushed the pyproject_support branch from 16c21e0 to 0014b5a Compare October 9, 2023 14:38

AnandInguva added 8 commits October 10, 2023 13:00

Add pyproject.toml

93d819a

Update numpy bounds

Use subprocess to run grpcio since it is not imported in pyproject.toml

4d29e90

Update setup.py Remove ImportError from gen_protos.py Update subprocess run and raise RuntimeError if proto generation fails Print output of setup.py Fix linting issues

Remove build-requirements.txt and use build to build the sdist

3d1d454

Modify buildPython task

Remove cython tests. default tests will run with Cython extensions

fb1f5a7

FIx toxTask name Add no-extra test suite to precommit and remove GH duplicate ubuntu test Skip failing non-cython test Fix tox test name

Force type cast inputs to list

d59c260

Update stager to use build. If it fails, use legacy setup to build sdist

4c80183

Fix mypy issue

Remove cython env and build-requirements for tox.ini

dad39ec

AnandInguva force-pushed the pyproject_support branch from 0014b5a to dad39ec Compare October 10, 2023 17:02

AnandInguva merged commit a94d29f into apache:master Oct 12, 2023
135 of 138 checks passed

AnandInguva added a commit that referenced this pull request Oct 13, 2023

Revert "Add pyproject.toml (#28385)"

566afcf

This reverts commit a94d29f.

AnandInguva mentioned this pull request Oct 13, 2023

Revert "Add pyproject.toml" #28993

Closed

chamikaramj mentioned this pull request Oct 16, 2023

[Failing Test]: JuliaSetTestIT.test_run_example_with_setup_file broken for Python post-commit test suites #29020

Closed

16 tasks

AnandInguva mentioned this pull request Oct 16, 2023

Log warning when gen_protos is not found #29025

Merged

3 tasks

Abacn mentioned this pull request Oct 19, 2023

[Failing Test]: Python ARM PostCommit failing after #28385 #29076

Closed

16 tasks

AnandInguva mentioned this pull request Oct 23, 2023

[Python]change extension of beam sdist from .zip to .tar.gz #29108

Merged

3 tasks

lazarillo reviewed Nov 9, 2023

View reviewed changes

This was referenced May 14, 2024

Don't build sdist or create a venv for tox tasks. #31290

Closed

Install Beam from wheels in Dependency Compat Test Suite. #31308

Merged

		@@ -103,6 +102,51 @@ def _load_or_default(filename):
		return {}


		_DESTINATION_ELEMENT_PAIRS = [

Add pyproject.toml #28385

Add pyproject.toml #28385

Conversation

AnandInguva commented Sep 9, 2023 • edited Loading

GitHub Actions Tests Status (on master branch)

AnandInguva commented Sep 9, 2023

codecov bot commented Sep 9, 2023 • edited Loading

Codecov Report

github-actions bot commented Sep 9, 2023

AnandInguva commented Sep 10, 2023

AnandInguva commented Sep 10, 2023

AnandInguva commented Sep 10, 2023

AnandInguva commented Sep 10, 2023

Choose a reason for hiding this comment

AnandInguva commented Sep 10, 2023

AnandInguva commented Sep 10, 2023

AnandInguva commented Sep 10, 2023

AnandInguva commented Sep 10, 2023

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

AnandInguva commented Oct 11, 2023

AnandInguva commented Oct 11, 2023

AnandInguva commented Oct 11, 2023

AnandInguva commented Oct 11, 2023

AnandInguva commented Oct 11, 2023

AnandInguva commented Oct 12, 2023

Abacn commented Oct 17, 2023

AnandInguva commented Oct 17, 2023

AnandInguva commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnandInguva commented Sep 9, 2023 •

edited

Loading

codecov bot commented Sep 9, 2023 •

edited

Loading