tvalentyn commented on code in PR #28385:
URL: https://github.com/apache/beam/pull/28385#discussion_r1340561540
##########
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:
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/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:
Let's update the comment. We should remove non-cython tests.
FYI I also reworded the issue.
##########
.github/workflows/beam_PreCommit_Python_Dataframes.yml:
##########
@@ -105,6 +105,7 @@ jobs:
arguments: |
-Pposargs=apache_beam/dataframe/ \
-PpythonVersion=${{ matrix.python_version }} \
+ -PuseWheelDistribution
Review Comment:
Please separate TOML change and Test runtime optimization into separate
commits after this review iteration.
##########
sdks/python/apache_beam/ml/gcp/naturallanguageml_test.py:
##########
@@ -60,12 +60,15 @@ def test_document_source(self):
self.assertFalse('content' in dict_)
self.assertTrue('gcs_content_uri' in dict_)
+ @unittest.skip(
+ 'TypeError: Expected bytes, got MagicMock.'
+ 'Please look at https://github.com/apache/beam/issues/28307.')
Review Comment:
Let's remove this test if we can't make it work. There is coverage of this
path in postcommit. Realistically, this won't get looked at most likely.
##########
sdks/python/apache_beam/runners/dataflow/internal/clients/cloudbuild/__init__.py:
##########
@@ -29,5 +29,3 @@
from
apache_beam.runners.dataflow.internal.clients.cloudbuild.cloudbuild_v1_messages
import *
except ImportError:
pass
-
Review Comment:
nit: FYI, you can remove the files from the PR altogether. You can `git
checkout $file` to revert to master version, and the commit changes.
##########
sdks/python/gen_protos.py:
##########
@@ -511,24 +472,23 @@ def generate_proto_files(force=False):
if not os.path.exists(PYTHON_OUTPUT_PATH):
os.mkdir(PYTHON_OUTPUT_PATH)
- grpcio_install_loc = ensure_grpcio_exists()
protoc_gen_mypy = _find_protoc_gen_mypy()
- with PythonPath(grpcio_install_loc):
+ try:
Review Comment:
this file is a bit hard to review. for changes like this, try to make
separate commits to make reviewing easier: separate formatting changes from
code changes, capture rationale in commit metadata.
--
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]
