Add common unshim packaging tooling [databricks] - #15025
Conversation
6d223b7 to
a0d7661
Compare
|
build |
…5-runtime-holdback-fix # Conflicts: # sql-plugin/src/main/spark330/scala/com/nvidia/spark/rapids/shims/Spark330PlusShims.scala # sql-plugin/src/main/spark330db/scala/com/nvidia/spark/rapids/shims/Spark330PlusDBShims.scala
|
build |
|
build |
1 similar comment
|
build |
|
Blossom is currently blocked by known issue #15293, not by this PR's current diff. The failing Blossom run hit This PR's effective diff is now infra/docs/build tooling only ( |
|
build |
|
NOTE: release/26.08 has been created from main. Please retarget your PR to release/26.08 if it should be included in the release. |
|
IMO, the PR stack(20+) is mainly a tool? From AI: The analyzer is a candidate generator, not a correctness proof. If it says "these 40 classes are root-safe" and it's wrong, the consequence isn't bad code landing — it's:
In other words, correctness comes from the downstream gate, not from the tool. Nobody reviews your grep invocations for the same reason. There's a nice inversion here: the moment you merge it and wire it into CI, it becomes a gate — and only then does it genuinely need review, because its correctness starts bearing load. So the act of merging is what creates the review burden; not merging eliminates it entirely. The current PR sits on the wrong side of that line: it packages a non-load-bearing heuristic as something reviewers are expected to vouch for — the Tarjan implementation, the constant-pool tag dispatch, the reversed edges in the Kahn sort. In practice no reviewer can verify any of that in reasonable time. The zero test coverage I flagged earlier is the symptom of exactly this mismatch. Migration PRs really are an order of magnitude cheaper to review Per batch, the reviewer only needs to check:
The first two are judgments humans are actually good at and must make; the third is the machine's job. None of it requires trusting the tool. Side benefit: the author iterates on the tool with zero process friction. Across 25 batches you inevitably learn and adjust as you go — if the tool lives in the repo, every improvement is an extra PR, and the hardest kind to review at that. One real risk, unrelated to tooling For this to work, every migration PR must build and test independently. But looking at the stack map, the dependency chain runs deep: #15026 wires modules → #15040/#15030 add helpers → #15043 et al. populate shim module sources → #15048 et al. migrate callers → #15035 et al. clean up old sources. If PRs 3 through 25 only make sense chained together and won't build standalone, then the "each PR is easy to review" premise is already discounted — and that has nothing to do with the tooling question. It's structural to the stack. Worth getting explicit confirmation from the author: after each PR merges to main individually, are build and tests green? If not, the fix isn't an infrastructure PR underneath — it's redrawing the batch boundaries. One concrete suggestion Put the evidence for exception entries in the PR description, rather than putting the tool in the repo. For example, when a migration PR needs an identity exception for RapidsErrorUtils, the author uses the local tool to generate a javap -c diff between the two shim versions, pastes it into the PR body, and states that the divergence is confined to Scala metadata and debug attributes with identical executable bytecode. The reviewer judges the evidence, not the tool's source. That's the right shape for this: output as evidence in the PR, not code as an asset in the repo. |
|
posted some comments from AI |
|
On keeping the tools in the repo: I think they should stay in this base PR because the refactor is intentionally gradual. The migration is not a one-shot local author workflow; follow-up PRs will need repeatable candidate generation, evidence collection, and packaging verification over time. Keeping the scripts in-tree makes that process reproducible for other contributors and reviewers, keeps the command line stable across batches, and avoids making one local checkout the hidden source of truth. The tools are still advisory where appropriate: the correctness boundary remains the packaging checks, class identity checks, CI, and the source diffs in each migration PR. |
|
build |
| for pat in glob_list: | ||
| new_matches = fnmatch.filter(namelist, pat) | ||
| matching_members += new_matches | ||
| glob_list = from_single_shim_or_each if bv == buildver_list[0] else from_each |
There was a problem hiding this comment.
AI pointed out that buildvers is sorted in build-unshim-parallel-world.py with reverse=True, but here we pick the 0th element of the item provided by user. Should we sort both lists the same way? Does it really matter?
Sorry no full context, AI and me could be wrong.
NvTimLiu
left a comment
There was a problem hiding this comment.
LGTM from CI point of view, for buildall script we'll nothing changed if we keep UNSHIM_PARALLEL_WORLD_ONLY unset
I'm not familiar with the paralele/dedupe related scripts.
Are we going to move all the common classes from spark-shared/com/nvidia to com/nvidia ?
yes, eventually after a series of refactoring PRs, all classes (99%) that remain in the sql-plugin module are proven to be Spark-version independent and can reside in the standard root part of the jar under com/nvidia, org/apache. Next PRs would work towards the new default UNSHIM_PROMOTE_DEFAULT_SPARK_SHARED_CLASSES=1 |
|
build |
|
build |
Related to #14834.
Description
This is the first PR in the unshim stack. The current take is to make this bottom layer infrastructure-only so it is reviewable and deliverable on its own.
This PR adds the packaging, diagnostics, build fast path, and documentation needed to evaluate common-class unshimming, but it does not move runtime/source classes and does not change normal production packaging behavior by default. The automatic promotion path for identical
spark-sharedclasses is opt-in for local validation withUNSHIM_PROMOTE_DEFAULT_SPARK_SHARED_CLASSES=1. With that variable unset, packaging continues to use the existing explicit promotion lists.Follow-up PRs in the stack will apply the source and packaging migrations in smaller themed batches.
Why this layer exists
The stack is moving toward an inverse unshim model: common bytecode that is identical across selected shims should be eligible for root-layout packaging, while true exceptions remain explicit.
This PR does not flip that policy on for normal builds. Instead, it adds the mechanics needed to review and validate the policy safely:
Keeping this PR infra-only avoids mixing the review of packaging mechanics with source-level bridge changes or runtime behavior changes.
What changes in this PR
dist/keep-in-spark-shared.txtfor classes that are identical but must remain underspark-shared.dist/keep-in-spark-shim-dirs.txtfor classes that must remain in selected Spark shim directories because they need same-loader visibility to shim-only classes.dist/scripts/analyze-parallel-world-deps.pyto inspect class-file dependencies and report static paths from root/common classes to version-specific shim bytecode.dist/scripts/build-unshim-parallel-world.pyto build/package a single-shim view cheaply while iterating on candidates.dist/scripts/binary-dedupe.shdiagnostics so the packaging result can be explained from class-file evidence.build/buildallfor repeated unshim iteration, including the cheaper fast path and-Ddist.jar.compress=falseon that path.docs/dev/shims.md,docs/dev/shimplify.md,docs/dev/parallel-world-unshimming.md, anddist/README.mdfor the new workflow.What is intentionally not in this PR
Reviewer guidance
Please review this as the infrastructure layer for the stack. The key questions are:
Stack map
Testing and validation notes
Local checks for the current infra-only shape:
git diff --checkbash -O extglob -n build/buildallbash -n dist/scripts/binary-dedupe.shpython3 -m py_compile dist/scripts/analyze-parallel-world-deps.py dist/scripts/build-unshim-parallel-world.py dist/build/package-parallel-worlds.pymvn verify -P individual,pre-merge -Dbuildver=330 -Ddist.jar.compress=false -DskipTests -Dmaven.scaladoc.skip -Dmaven.artifact.threads=10 --batch-mode -Dmaven.wagon.http.retryHandler.count=3 -Dmaven.wagon.httpconnectionManager.ttlSeconds=30 -Daether.connector.http.connectionMaxTtl=30Checklists
Documentation
Testing
(Covered by packaging/no-tests build validation and syntax/format checks for the new tooling.)
Performance