from_protobuf: Re-enable protobuf integration tests on OSS Spark and Databricks [databricks] [fast-ut] [reduced-it] - #15814
Conversation
…VIDIA#14885) Part of NVIDIA#14069. First slice carved out of NVIDIA#14354 following the plugin-side split plan in the issue. ### Description #### Problem `from_protobuf` is provided by Spark's optional `spark-protobuf` module, which is not normally present on the integration-test classpath. Subsequent GPU-support PRs need a stable CPU fallback baseline without introducing an unshaded protobuf runtime dependency. #### Changes - Copy `spark-protobuf_${scala.binary.version}` into the integration-test dependencies for Spark 3.4+, for both Scala 2.12 and 2.13 builds. Spark 3.3 profiles skip this artifact. - Add `INCLUDE_SPARK_PROTOBUF_JAR` handling to `run_pyspark_from_build.sh`. Protobuf tests are enabled by default when exactly one matching jar is found: - one jar: add it to `ALL_JARS`; - no jar: disable the tests, warning when inclusion was explicitly requested; - multiple jars: warn and disable the tests to avoid classpath-dependent API selection; - `INCLUDE_SPARK_PROTOBUF_JAR=false`: disable the tests without a warning. - Add a driver-local `local_tmp_path` fixture. The path-based protobuf API reads its descriptor with `java.io.File`, so a Hadoop-backed temporary path is not suitable on distributed filesystems. - Add two CPU-fallback smoke tests: - `test_from_protobuf_smoke_path_api` for Spark 3.4+; - `test_from_protobuf_smoke_binary_descriptor_api` for Spark 3.5+, skipped when the PySpark API does not expose `binaryDescriptorSet`. The tests use static `FileDescriptorSet` bytes and hand-encoded messages. This avoids depending on whichever unshaded protobuf runtime the Spark driver provides. The path-based test writes those bytes to `local_tmp_path`; the Spark 3.5+ test passes the same bytes directly. No GPU implementation is added. Both tests use `assert_gpu_fallback_collect("ProtobufDataToCatalyst")` to compare CPU and GPU-mode results while verifying CPU fallback. #### Testing - Spark 3.5.2 targeted integration run: - `test_from_protobuf_smoke_path_api` - `test_from_protobuf_smoke_binary_descriptor_api` - Result: `2 passed` - Spark 3.4.1 standalone compatibility probe: the path-based API successfully read the static descriptor; the binary descriptor API is not available in Spark 3.4. - Multiple matching `spark-protobuf` jars: warning emitted, jars excluded, and protobuf tests skipped. - `bash -n`, `shellcheck -S error`, and `git diff --check` passed. ### Checklists Documentation - [ ] Updated for new or modified user-facing features or behaviors - [x] No user-facing change Testing - [x] Added or modified tests to cover new code paths - [ ] Covered by existing tests (Please provide the names of the existing tests in the PR description.) - [ ] Not required Performance - [ ] Tests ran and results are added in the PR description - [ ] Issue filed with a link in the PR description - [x] Not required --------- Signed-off-by: Haoyang Li <haoyangl@nvidia.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
thirtiseven
changed the title
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
|
build |
|
Is this split of the old PR #14354 ? |
Yes it is. |
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Greptile SummaryThis PR restores CPU-fallback integration coverage for Spark’s
Confidence Score: 4/5The PR should not merge until Databricks integration-test packaging receives the protobuf-copy skip and no longer resolves an unavailable vendor-versioned artifact. The Databricks Maven reactor packages integration_tests, but the new skip is confined to a parent profile whose effects are not inherited into that child model, leaving external protobuf copying enabled for vendor Spark versions. Files Needing Attention: shim-deps/pom.xml, scala2.13/shim-deps/pom.xml, pom.xml, scala2.13/pom.xml Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Build[Maven package] --> Runtime{Runtime profile}
Runtime -->|OSS Spark 3.4+| Copy[Copy matching spark-protobuf jar]
Runtime -->|Databricks| Bundled[Use runtime-bundled protobuf API]
Copy --> Runner[run_pyspark_from_build.sh]
Bundled --> Runner
Runner --> Tests[Pytest protobuf smoke tests]
Tests --> CPU[CPU reference execution]
Tests --> GPU[GPU-enabled execution]
GPU --> Fallback[Verify ProtobufDataToCatalyst fallback]
CPU --> Compare[Compare decoded rows]
Fallback --> Compare
Reviews (1): Last reviewed commit: "Clarify protobuf test eligibility" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
build |
This is what I got from the pre-merge CI: 
|
amahussein
left a comment
amahussein
left a comment
There was a problem hiding this comment.
Thanks @thirtiseven
My comments are not blockers.
One ask, inline: the default configuration skips silently. No jar and INCLUDE_SPARK_PROTOBUF_JAR unset gives 2 skipped, exit 0, no warning. That is NvTimLiu's second question, and widening the existing warning by one condition answers it in code.
Follow-ups, not merge conditions:
- Neither test asserts a decoded value, so a wire-valid encoding error passes. An off-by-one mutant on the encoder still gave 2 passed. Worth pinning the four known rows in the follow-up that adds semantic coverage.
- The Python
protobufrequirement is unused by this diff. - The branch is 34 commits behind
mainand does not compile standalone against the current JNI snapshot (unrelatedapplyBooleanMaskdeprecations fixed by #15848). CI tests the merge result so pre-merge is green, but a merge frommainbefore this lands would keep a plain checkout of the branch buildable.
| elif [[ "$INCLUDE_SPARK_PROTOBUF_JAR_REQUESTED" == "true" ]]; then | ||
| >&2 echo "WARNING: INCLUDE_SPARK_PROTOBUF_JAR=true was requested but a spark-protobuf jar was not found (searched: $PROTOBUF_JARS)." |
There was a problem hiding this comment.
This is the default configuration and it fails silently. With no jar on disk and INCLUDE_SPARK_PROTOBUF_JAR unset, neither warning fires (the first needs >1, this one needs an explicit true), so both tests skip and the run exits 0 with no diagnostic. I reproduced it by removing the jar: 2 skipped, rc=0, no log line matching warning.*protobuf. Since injection is opt-out, silence should be reserved for the case where someone actually opted out. This also answers NvTimLiu's question about nightly runs where $LOCAL_JAR_PATH/spark-protobuf*.jar is absent.
| elif [[ "$INCLUDE_SPARK_PROTOBUF_JAR_REQUESTED" == "true" ]]; then | |
| >&2 echo "WARNING: INCLUDE_SPARK_PROTOBUF_JAR=true was requested but a spark-protobuf jar was not found (searched: $PROTOBUF_JARS)." | |
| elif [[ "$INCLUDE_SPARK_PROTOBUF_JAR_REQUESTED" != "false" ]]; then | |
| >&2 echo "WARNING: a spark-protobuf jar was not found (searched: $PROTOBUF_JARS); protobuf tests will be skipped." |
| return _make_smoke_df(spark).select( | ||
| from_protobuf_fn(f.col("bin"), "test.Simple", desc_path).alias("d")) | ||
|
|
||
| assert_gpu_fallback_collect(run, "ProtobufDataToCatalyst") |
There was a problem hiding this comment.
Both tests compare CPU against GPU-with-fallback, and ProtobufDataToCatalyst always falls back, so both sides run the same CPU implementation and no decoded value is ever asserted. I checked what that costs by mutating the encoder to _encode_varint(i32_value + 1): still 2 passed. To be fair the tests are not vacuous, since FAILFAST means a malformed payload or a mismatched descriptor does fail them (I verified both), and the fallback assertion is real. But for a baseline that later GPU slices get diffed against, the values are the part worth pinning. Could the follow-up assert the four known _smoke_rows against the decoded struct?
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| pytest | ||
| protobuf |
There was a problem hiding this comment.
Nothing in this diff imports protobuf; the tests deliberately use a static hex descriptor to avoid the dependency. Since the images have to be rebuilt after merge before it is even available, it seems to belong in the follow-up that needs it rather than here.
4 participants
Part of #14069.
Description
#14885 added the first
from_protobufintegration-test infrastructure slice, but it was reverted by #15243 because Databricks builds attempted to resolve unpublished vendor-versionedspark-protobufartifacts. The follow-up discussion in #15232 also established that disabling external jar injection must not disable the tests on Databricks runtimes, where DBR 12.2+ bundles the API.This PR restores that reviewed infrastructure and corrects the runtime handling:
spark-protobufjar for supported Apache Spark 3.4+ builds;INCLUDE_SPARK_PROTOBUF_JARto control external-jar injection and Apache Spark test eligibility while detecting Databricks test eligibility independently;protobufpackage in the integration-test image dependencies for follow-up semantic coverage; andfrom_protobufis unavailable.No GPU implementation is enabled in this PR. This only establishes dependency handling and the CPU-fallback baseline for later slices. The new Python dependency becomes available to regular CI after this PR is merged and the integration-test images are rebuilt. Performance testing is not required because plugin runtime code is unchanged.
AI assistance: OpenAI Codex was used to help implement and review this change; the author reviewed the final diff and description.
Checklists
Documentation
Testing
(Please provide the names of the existing tests in the PR description.)
Performance