Yikun commented on a change in pull request #33591:
URL: https://github.com/apache/spark/pull/33591#discussion_r680285100
##########
File path: .github/workflows/build_and_test.yml
##########
@@ -41,8 +47,11 @@ jobs:
build:
name: "Build modules (${{ format('{0}, {1} job',
needs.configure-jobs.outputs.branch, needs.configure-jobs.outputs.type) }}):
${{ matrix.modules }} ${{ matrix.comment }} (JDK ${{ matrix.java }}, ${{
matrix.hadoop }}, ${{ matrix.hive }})"
needs: configure-jobs
- # Do not run as scheduled jobs in forked repos
- if: github.repository == 'apache/spark' ||
needs.configure-jobs.outputs.type == 'regular'
+ # Run scheduled jobs for Apache Spark only
+ # Run regular jobs for commit in both Apache Spark and fored repository
Review comment:
nit: s/fored/forked/
##########
File path: .github/workflows/build_and_test.yml
##########
@@ -251,9 +264,20 @@ jobs:
bash miniconda.sh -b -p $HOME/miniconda
# Run the tests.
- name: Run tests
+ env: ${{ fromJSON(needs.configure-jobs.outputs.envs) }}
run: |
+ # TODO(SPARK-36361): Install coverage in Python 3.9 and PyPy 3 in the
base image
+ python3.9 -m pip install coverage
+ pypy3 -m pip install coverage
export PATH=$PATH:$HOME/miniconda/bin
./dev/run-tests --parallelism 1 --modules "$MODULES_TO_TEST"
+ - name: Upload coverage to Codecov
+ if: needs.configure-jobs.outputs.type == 'pyspark-coverage-scheduled'
+ uses: codecov/codecov-action@v1
Review comment:
qq: Just not sure what's the diff between v1 and v2, looks like v2 is
the recommand version in codecov-action.
[1] https://github.com/codecov/codecov-action#usage
##########
File path: .github/workflows/build_and_test.yml
##########
@@ -251,9 +264,20 @@ jobs:
bash miniconda.sh -b -p $HOME/miniconda
# Run the tests.
- name: Run tests
+ env: ${{ fromJSON(needs.configure-jobs.outputs.envs) }}
run: |
+ # TODO(SPARK-36361): Install coverage in Python 3.9 and PyPy 3 in the
base image
+ python3.9 -m pip install coverage
Review comment:
nit: I guess we could also introduce the coverage in
https://github.com/apache/spark/blob/master/dev/requirements.txt
##########
File path: python/test_coverage/coverage_daemon.py
##########
@@ -17,26 +17,29 @@
import os
import imp
+import platform
# This is a hack to always refer the main code rather than built zip.
main_code_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
daemon = imp.load_source("daemon", "%s/pyspark/daemon.py" % main_code_dir)
if "COVERAGE_PROCESS_START" in os.environ:
- worker = imp.load_source("worker", "%s/pyspark/worker.py" % main_code_dir)
+ # PyPy with coverage makes the tests flaky, and CPython is enough for
coverage report.
+ if "pypy" not in platform.python_implementation().lower():
Review comment:
nit: The `platform.python_implementation()` returns one of values
(‘CPython’, ‘IronPython’, ‘Jython’, ‘PyPy’) rather than a list, so I guess `!=`
is engough. or do you have any other concern?
##########
File path: dev/run-tests.py
##########
@@ -815,11 +765,10 @@ def main():
modules_with_python_tests = [m for m in test_modules if
m.python_test_goals]
if modules_with_python_tests:
- # We only run PySpark tests with coverage report in one specific job
with
- # Spark master with SBT in Jenkins.
- is_sbt_master_job = "SPARK_MASTER_SBT_HADOOP_2_7" in os.environ
run_python_tests(
- modules_with_python_tests, opts.parallelism,
with_coverage=is_sbt_master_job)
+ modules_with_python_tests,
+ opts.parallelism,
+ os.environ.get("PYSPARK_CODECOV", "false") == "true")
Review comment:
nit: it's better to stay the explict args name for python default value
argument, this is a style prefer for me, just feel free to change or not. : )
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
