dongjoon-hyun commented on PR #58274:
URL: https://github.com/apache/spark/pull/58274#issuecomment-5417968562
Thanks for the cleanup! The rename itself looks mechanically complete -- I
grepped for any leftover `pyspark-install` / `pyspark_install` reference and
found none outside the four files you touched, and the updated
`dev/sparktestsupport/utils.py` doctests pass locally.
One thing I'd like to discuss before this goes in.
### Moving the upstream PyArrow tests out of `pyspark-core` drops them from
the scheduled version matrix
`pyspark-core` runs in every workflow that passes `"pyspark": "true"`, but
`pyspark-scheduled` does not. Since this PR only updates `build_coverage.yml`,
the 13 upstream PyArrow test goals stop running in these scheduled workflows:
`build_python_minimum`, `build_python_3.11`, `build_python_3.13`,
`build_python_3.14`, `build_python_3.14_nogil`, `build_python_3.12_pandas_3`,
`build_python_3.12_classic_only`, `build_java21`, `build_java25`,
`build_non_ansi`, `build_uds`, `build_rockdb_as_ui_backend`, `build_codegen_jdk`
They also disappear from the ARM and macOS daily builds, because
`.github/workflows/python_hosted_runner_test.yml` names `pyspark-core` directly
in its matrix:
```yaml
modules:
- >-
pyspark-sql, pyspark-resource, pyspark-testing
- >-
pyspark-core, pyspark-errors, pyspark-streaming
```
That is the part I'm unsure about. These tests are written specifically for
that version matrix -- they carry explicit `LooseVersion(pa.__version__)` /
`LooseVersion(pd.__version__)` branches and per-version overrides, and
`goldenutils.py` states the goal outright: "so that the same golden file works
for multiple versions". `build_python_minimum` (oldest supported
PyArrow/pandas) and `build_python_3.14` / `build_python_3.12_pandas_3` (newest)
are exactly where an upstream behavior change would surface first, and those
are the runs losing the coverage.
So after this PR, "we run these on scheduled CI" effectively means
`build_coverage` only.
Two ways forward:
1. Add `"pyspark-scheduled": "true"` to the `jobs` input of the workflows
above, and add the module to `python_hosted_runner_test.yml`. The downside is
that this also flips `SKIP_PACKAGING=false` (build_and_test.yml, "Reusing
precompiled artifact" step), so every one of those runs picks up the slow
install/packaging tests too.
2. Split the module instead: keep the install/packaging tests in
`pyspark-scheduled` and put the upstream monitoring tests in their own module
(e.g. `pyspark-upstream`). Then the version-matrix workflows can opt into the
upstream tests without paying for packaging. This also seems closer to the
categorization argument in the PR description -- the two groups are "not
sensitive to pyspark changes" for quite different reasons, and they want
different CI placement.
I lean toward (2), but either is fine with me. If the reduced coverage is
intentional, could you note the reasoning in the PR description?
### Minor
The new comment in `build_and_test.yml` is slightly off:
```yaml
# pyspark-scheduled will only run when explicitly requested, it contains
tests that are not sensitive to pyspark code changes.
```
It is not only when explicitly requested -- on fork pre-merge CI it is still
triggered by `is-changed.py` whenever `python/` changes, which is why the
`Build modules: pyspark-scheduled` job ran on this very PR. The previous
wording ("when it's changed or explicitly requested") was accurate. While
there, the line is 135 characters; wrapping it would match the surrounding
comments.
Same nit on the name: `scheduled` reads as "runs on the scheduled CI", but
in practice this module runs mostly on pre-merge. Not a blocker, though if you
go with the module split above, something like `pyspark-packaging` /
`pyspark-upstream` would describe each group more directly.
--
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]
