damccorm commented on code in PR #23451: URL: https://github.com/apache/beam/pull/23451#discussion_r1024344413
########## .github/workflows/job-precommit-python.yml: ########## @@ -0,0 +1,275 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: PreCommit Python + +on: + workflow_dispatch: + schedule: + - cron: '0 */6 * * *' + push: + branches: ['master', 'release-*'] + tags: 'v*' + pull_request_target: + branches: [ 'master', 'release-*'] + tags: 'v*' + paths: + - 'sdks/python/apache_beam/**' + - 'sdks/python/apache_beam/coders/**' + - 'sdks/python/apache_beam/internal/**' + - 'sdks/python/apache_beam/io/concat_source_test.py' + - 'sdks/python/apache_beam/io/external/generate_sequence_test.py' + - 'sdks/python/apache_beam/io/hadoopfilesystem_test.py' + - 'sdks/python/apache_beam/io/iobase_test.py' + - 'sdks/python/apache_beam/io/range_trackers_test.py' + - 'sdks/python/apache_beam/io/restriction_trackers_test.py' + - 'sdks/python/apache_beam/io/source_test_utils_test.py' + - 'sdks/python/apache_beam/io/sources_test.py' + - 'sdks/python/apache_beam/io/utils_test.py' + - 'sdks/python/apache_beam/io/watermark_estimators_test.py' + - 'sdks/python/apache_beam/metrics/**' + - 'sdks/python/apache_beam/options/**' + - 'sdks/python/apache_beam/runners/common_test.py' + - 'sdks/python/apache_beam/runners/pipeline_context_test.py' + - 'sdks/python/apache_beam/runners/portability/artifact_service_test.py' + - 'sdks/python/apache_beam/runners/portability/fn_api_runner/**' + - 'sdks/python/apache_beam/runners/portability/job_server_test.py' + - 'sdks/python/apache_beam/runners/portability/local_job_service_test.py' + - 'sdks/python/apache_beam/runners/portability/portable_runner_test.py' + - 'sdks/python/apache_beam/runners/portability/sdk_container_builder_test.py' + - 'sdks/python/apache_beam/runners/portability/stager_test.py' + - 'sdks/python/apache_beam/runners/runner_test.py' + - 'sdks/python/apache_beam/runners/sdf_utils_test.py' + - 'sdks/python/apache_beam/runners/worker/**' + - 'sdks/python/apache_beam/testing/**' + - 'sdks/python/apache_beam/tools/microbenchmarks_test.py' + - 'sdks/python/apache_beam/transforms/**' + - 'sdks/python/apache_beam/typehints/**' + - 'sdks/python/apache_beam/utils/**' +permissions: read-all + +jobs: + set-properties: + runs-on: [self-hosted, ubuntu-20.04] + outputs: + properties: ${{ steps.test-properties.outputs.properties }} + steps: + - name: Checkout code + uses: actions/checkout@v3 + with: + ref: ${{ github.event.pull_request.head.sha }} + persist-credentials: false + - id: test-properties + uses: ./.github/actions/setup-default-test-properties + + precommit-python: + needs: set-properties + name: PreCommit Python + runs-on: [self-hosted, ubuntu-20.04] + strategy: + fail-fast: false + matrix: + version: ${{fromJson(needs.set-properties.outputs.properties).PythonTestProperties.ALL_SUPPORTED_VERSIONS}} + tox-env: ${{fromJson(needs.set-properties.outputs.properties).PythonTestProperties.TOX_ENV}} + steps: + - name: Checkout code + uses: actions/checkout@v3 + with: + ref: ${{ github.event.pull_request.head.sha }} + persist-credentials: false + - name: Set python version + run: echo "PYTHON_VERSION=$(echo ${{ matrix.version }} | sed -e 's/\.//g')" >> $GITHUB_ENV + - name: Setup environment + uses: ./.github/actions/setup-self-hosted-action + with: + requires-go-18: false + + - name: Run Apache Beam Tests + uses: ./.github/actions/gradle-command-self-hosted-action + with: + gradle-command: :sdks:python:test-suites:tox:py${{env.PYTHON_VERSION}}:testPy${{env.PYTHON_VERSION}}${{matrix.tox-env}} + arguments: "-Pposargs=apache_beam/" Review Comment: I was revisiting this and realized I don't think its actually working the way we expect it to. A couple notes: 1) This runs the tests for all of the apache_beam directory. This means that subsequent steps are just trying to retest things tested here. Since that gets cached, subsequent steps will do nothing. See https://github.com/roger-mike/beam/actions/runs/3161439176/jobs/5147285589 which takes 10 minutes for this step and 10 seconds for each subsequent step. 2) As best I can tell `gradlew :sdks:python:test-suites:tox:py${{env.PYTHON_VERSION}}:testPy${{env.PYTHON_VERSION}}${{matrix.tox-env}} <path/to/tests>` doesn't actually limit runs to tests in that directory. So I don't think this approach is generally valid. 3) (least important, but still matters) Sharding is most helpful when things are split into different jobs, not just different steps. This kind of sharding doesn't really buy us much. Sharding into jobs would also make some of these problems more obvious. -- 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]
