Assert RapidsDeltaWrite in integration tests [databricks] - #13586
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds assertion functionality to detect when Delta Lake writes are executed on GPU versus CPU in integration tests. The primary purpose is to catch unintended CPU fallback scenarios that cannot be detected through traditional query plan validation.
Key changes:
- New
assert_rapids_delta_writefunction that captures execution plans and validates presence ofRapidsDeltaWrite - Automatic detection and application of Delta write validation for tests with
@delta_lakemarker - Updates to Delta test functions to use the new assertion mechanism for proper GPU validation
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
ExecutionPlanCaptureCallback.scala |
Adds contains method interface to check for specific class names in execution plans |
ShimmedExecutionPlanCaptureCallbackImpl.scala |
Implements the contains method and updates copyright year |
delta_lake_utils.py |
Implements assert_rapids_delta_write function with plan capture and validation logic |
asserts.py |
Modifies write assertion functions to automatically apply Delta validation for Delta tests |
conftest.py |
Adds global tracking of @delta_lake marker for current test |
| Various test files | Updates test functions to use new assertion mechanism and adds xfail markers |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
This change revealed that the insert overwrite support with dynamic partition overwrite (#13110) is missing not only for clustered tables but also for regular tables. |
|
build |
razajafri
left a comment
There was a problem hiding this comment.
Just some minor comments
The CI failure was due to the bind failure in starting the spark connect server. Not sure why it failed though. Re-running the CI in case it was some intermittent issue. |
|
build |
Some tests failed on databricks. Looking into it. |
|
The test failures are because of #11169. I xfailed them. |
|
build |
1 similar comment
|
build |
|
CI is failing on Databricks You can use the jobs-for-developers to test all Databricks instances against your branch at the same time before re-running the CI, it might save us time |
Signed-off-by: Jihoon Son <ghoonson@gmail.com>
|
build |
|
This is interesting. The Ci has passed now without any fix for the previous build failure. I suspect some intermittent issue. I pushed one more commit to remove xfail marks for #13110 which has been resolved now. Also I rebased my branch and force pushed because I messed up my branch while merging the upstream. Apology for any inconvenience in the review. |
I also used an Nvidia's internal jenkins job to run the build and the integration tests on databricks 12.2, 13.3. and 14.3 as @razajafri suggested above. |
gerashegalov
left a comment
There was a problem hiding this comment.
LGTM
I think we might have an issue in general that we don't register sub-executions for some Delta operations which makes it hard to navigate to underlying plans
Yeah, maybe we should extend this approach later as a follow-up to always capture all executions for Delta tests and verify whether they are on GPU. |
Fixes #13059
Description
In integration tests, we can detect plans executed on CPU (so-called CPU fallback) to catch unintended execution on CPU. Every operator is expected to run on GPU unless it is explicitly specified in the
allow_non_gpumarker. This works well in most cases, but has limitations:Due to these limitations, we currently cannot validate the delta lake writes. The delta lake writes usually do not appear in the query plan. Also, they are oftentimes directly created and executed in various Delta commands without going through the optimizer. As such, we had missed bugs in the past that the write is performed on CPU for certain Delta commands.
To address this issue, this PR adds a new function
assert_rapids_delta_writeindelta_lake_utils.py. This function executes the given function and captures all plans executed during executing the function. Once the execution is finished, it iterates over all captured plans to find theRapidsDeltaWrite, which must exist if the write was performed on GPU. Since Delta commands usually execute more than one plans,assert_rapids_delta_writeassumes the plan is valid ifRapidsDeltaWriteis found in at least one captured plan.To avoid invasive change, the
assert_gpu_and_cpu_writes_are_equal_collectandassert_gpu_and_cpu_save_as_table_are_equal_collectfunctions are modified to detect whether the current running test is a delta test and call theassert_rapids_delta_writefunction. There are some custom assert functions as well in some command tests. Those custom functions are also modified to callassert_rapids_delta_write. I manually confirmed all delta tests run successfully with Spark 3.5.5. Also verified some non-delta tests passing (parquet_write_test.py) as well.Checklists
(Please explain in the PR description how the new code paths are tested, such as names of the new/existing tests that cover them.)