AnandInguva commented on code in PR #28385:
URL: https://github.com/apache/beam/pull/28385#discussion_r1322080707
##########
.github/workflows/typescript_tests.yml:
##########
@@ -89,10 +89,8 @@ jobs:
- name: Setup Beam Python
working-directory: ./sdks/python
run: |
- pip install pip setuptools --upgrade
- pip install -r build-requirements.txt
pip install 'pandas>=1.0,<1.5'
- python setup.py develop
+ python install -e .
Review Comment:
```suggestion
pip install -e .
```
##########
sdks/python/scripts/run_pytest.sh:
##########
@@ -42,10 +42,10 @@ echo "posargs: $posargs"
# Run with pytest-xdist and without.
pytest -o junit_suite_name=${envname} \
- --junitxml=pytest_${envname}.xml -m 'not no_xdist' -n 6 ${pytest_args}
--pyargs ${posargs}
+ --junitxml=pytest_${envname}.xml -m 'not no_xdist' -n 6
--import-mode=importlib ${pytest_args} --pyargs ${posargs}
Review Comment:
No, that case works as expected.
##########
.github/workflows/beam_PreCommit_Python_Dataframes.yml:
##########
@@ -105,6 +105,7 @@ jobs:
arguments: |
-Pposargs=apache_beam/dataframe/ \
-PpythonVersion=${{ matrix.python_version }} \
+ -PuseWheelDistribution
Review Comment:
For PreCommit tests, we will use the wheel built by cibuildwheel instead of
tox installing apache beam from sources(which is taking ~2/3 minutes longer
than the legacy setup.py). This way reduces the time takes to run unit tests
##########
sdks/python/apache_beam/io/gcp/bigquery_test.py:
##########
@@ -103,6 +102,51 @@ def _load_or_default(filename):
return {}
+_DESTINATION_ELEMENT_PAIRS = [
Review Comment:
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.
##########
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"]
- }
+ }
Review Comment:
```suggestion
}
```
##########
sdks/python/setup.py:
##########
@@ -213,22 +242,9 @@ def get_portability_package_data():
*get_portability_package_data()
]
},
- ext_modules=cythonize([
- 'apache_beam/**/*.pyx',
- 'apache_beam/coders/coder_impl.py',
- 'apache_beam/metrics/cells.py',
- 'apache_beam/metrics/execution.py',
- 'apache_beam/runners/common.py',
- 'apache_beam/runners/worker/logger.py',
- 'apache_beam/runners/worker/opcounters.py',
- 'apache_beam/runners/worker/operations.py',
- 'apache_beam/transforms/cy_combiners.py',
- 'apache_beam/transforms/stats.py',
- 'apache_beam/utils/counters.py',
- 'apache_beam/utils/windowed_value.py',
- ],
- language_level=3),
+ ext_modules=extensions,
install_requires=[
+ 'build>=0.9.0,<0.11.0',
Review Comment:
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/apache_beam/coders/slow_coders_test.py:
##########
@@ -25,14 +25,17 @@
from apache_beam.coders.coders_test_common import *
[email protected](
+ 'Add a test for non-compiled implementation.'
+ 'https://github.com/apache/beam/issues/28307')
Review Comment:
Sg. I will follow up in a different PR
##########
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy:
##########
@@ -3032,30 +3032,40 @@ class BeamModulePlugin implements Plugin<Project> {
}
return argList.join(' ')
}
-
project.ext.toxTask = { name, tox_env, posargs='' ->
project.tasks.register(name) {
dependsOn setupVirtualenv
dependsOn ':sdks:python:sdist'
-
- doLast {
- // Python source directory is also tox execution workspace, We want
- // to isolate them per tox suite to avoid conflict when running
- // multiple tox suites in parallel.
- project.copy { from project.pythonSdkDeps; into copiedSrcRoot }
-
- def copiedPyRoot = "${copiedSrcRoot}/sdks/python"
- def distTarBall = "${pythonRootDir}/build/apache-beam.tar.gz"
- 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')) {
Review Comment:
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.
##########
sdks/python/apache_beam/examples/kafkataxi/README.md:
##########
@@ -157,9 +157,9 @@ Install Beam and dependencies and build a Beam distribution.
```sh
cd beam/sdks/python
-pip install -r build-requirements.txt
pip install -e '.[gcp]'
-python setup.py sdist
+pip install -q build
Review Comment:
I don't think so. Build is required for building wheels and sdist.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
