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: 5/5The PR appears safe to merge; no actionable correctness, security, build-profile, or repository-rule issue remains. The current profiles avoid resolving protobuf artifacts where they are unavailable, the launcher keeps OSS test eligibility synchronized with actual jar injection, and supported Databricks runs use the bundled API. The previous Maven inheritance concern was correctly withdrawn after effective-model verification and is resolved. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Test launcher] --> B{Databricks runtime?}
B -- Yes --> C[Disable external protobuf jar injection]
C --> D[Use runtime-bundled protobuf API]
B -- No --> E{Spark 3.4+ and exactly one matching jar?}
E -- Yes --> F[Inject spark-protobuf jar]
E -- No --> G[Disable protobuf tests]
D --> H[Run descriptor path and binary descriptor smoke tests]
F --> H
H --> I[Compare CPU and GPU runs]
I --> J[Verify ProtobufDataToCatalyst CPU fallback]
Reviews (3): Last reviewed commit: "Merge branch 'main' into protobuf-test-i..." | 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." |
There was a problem hiding this comment.
Fixed in ff62aca. A missing jar now warns unless INCLUDE_SPARK_PROTOBUF_JAR=false is explicitly set.
| 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?
There was a problem hiding this comment.
Yes. This PR only adds smoke coverage for the plugin-0 path. The follow-up semantic tests will compare the decoded rows with _smoke_rows.
| # 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.
There was a problem hiding this comment.
We need this dependency here so the CI images are ready for the follow-up. The images install requirements.txt only when they are built. If we add protobuf with the semantic tests, that PR will still use an older image without it. The Python package will be used by those follow-up tests.
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
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