fix: diagnose LightGBM reproducibility mismatches - #2663
Open
Rana Singh (ranadeepsingh) wants to merge 5 commits into
Open
fix: diagnose LightGBM reproducibility mismatches#2663Rana Singh (ranadeepsingh) wants to merge 5 commits into
Rana Singh (ranadeepsingh) wants to merge 5 commits into
Conversation
## Summary Add targeted LightGBM reproducibility warnings, documentation, and a public-estimator Parquet round-trip regression covering rows, metadata, vector representation, model dumps, predictions, and AUC. ## Prompting Intent Investigate GitHub issue microsoft#2394 end-to-end on Spark 3.5, distinguish input and configuration nondeterminism from a SynapseML serialization defect, avoid changing training semantics or performance without evidence, and provide a before/after regression plus actionable diagnostics. ## Linked Sources - GitHub issue: microsoft#2394 - LightGBM deterministic parameter documentation: https://lightgbm.readthedocs.io/en/latest/Parameters.html#deterministic - LightGBM determinism discussion: lightgbm-org/LightGBM#3761 - LightGBM row-order discussion: lightgbm-org/LightGBM#6320 ## Rationale The regression found exact parity between in-memory and Parquet-reloaded inputs and outputs when LightGBM's documented deterministic contract is followed. Rather than sorting, repartitioning, caching, or altering estimator defaults, warn only for effective deterministic training with nondeterministic Spark lineage or an incomplete/conflicting histogram configuration, and document snapshot materialization and training-overfit interpretation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey Rana Singh (@ranadeepsingh) 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Rana Singh (ranadeepsingh)
marked this pull request as ready for review
August 18, 2026 18:59
Rana Singh (ranadeepsingh)
requested review from
Brendan Walsh (BrendanWalsh) and
Scott Votaw (svotaw)
as code owners
August 18, 2026 18:59
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
August 18, 2026 18:59
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds diagnostics and documentation to help users understand and debug LightGBM “reproducibility” mismatches that stem from nondeterministic Spark lineage and incomplete deterministic LightGBM configuration, and introduces a regression suite that validates Parquet round-trip parity for LightGBMClassifier training inputs/outputs.
Changes:
- Add reproducibility warning logic to LightGBM training when deterministic mode is enabled.
- Add a comprehensive Parquet round-trip reproducibility regression suite for LightGBMClassifier.
- Improve pass-through parameter parsing utilities and document recommended deterministic training practices.
Show a summary per file
| File | Description |
|---|---|
| lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/LightGBMBase.scala | Logs reproducibility warnings for deterministic training (Spark lineage + histogram strategy). |
| lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/LightGBMUtils.scala | Adds helpers to extract pass-through parameter values and recognize LightGBM “true” boolean values. |
| lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/VerifyLightGBMCommon.scala | Adds a unit test asserting boolean pass-through parsing behavior matches expected LightGBM syntax. |
| lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/LightGBMParquetReproducibilitySuite.scala | New regression suite validating Parquet row/metadata parity and deterministic model/prediction stability. |
| docs/Explore Algorithms/LightGBM/Overview.md | Documents deterministic training requirements and the need to materialize a single Spark snapshot. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
## Summary Restrict deterministic histogram guidance to effective CPU training, materialize and clean up one stable in-memory snapshot in the Parquet regression, and allocate the suite's LightGBM listen port dynamically. ## Prompting Intent Resolve all three blocking Copilot review threads on PR microsoft#2663 without changing estimator defaults: honor typed and pass-through device precedence, keep nondeterministic-lineage coverage separate from stable parity proof, prevent parallel port collisions, and validate the changes across Spark 3.5 and Spark 4.1. ## Linked Sources - Pull request: microsoft#2663 - CPU warning review: microsoft#2663 (comment) - Snapshot materialization review: microsoft#2663 (comment) - Dynamic port review: microsoft#2663 (comment) ## Rationale Reuse the existing effective-device resolver so passThroughArgs retains precedence over typed device parameters. MEMORY_AND_DISK plus an eager count creates one exact snapshot while try/finally guarantees unpersist cleanup; the separate warning test retains the unseeded lineage. Reusing getAndIncrementPort follows existing suite conventions without changing product networking behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Canonicalize and materialize the in-memory and Parquet training layouts before comparing native models, predictions, and AUC in the reproducibility regression. ## Prompting Intent Keep the regression's exact input snapshot and cleanup guarantees while making its model comparison valid on both Spark 3.5 and Spark 4.1, where a Parquet read can change physical row and partition order independently of serialized values or metadata. ## Linked Sources - Pull request: microsoft#2663 - Snapshot materialization review: microsoft#2663 (comment) ## Rationale Spark 4.1 replay showed stable repeated models within each source but different models and predictions across storage layouts despite identical rows, metadata, and AUC. Hash repartitioning by the stable row ID plus per-partition sorting is confined to the test and isolates Parquet encoding without changing product semantics. Both canonical inputs use MEMORY_AND_DISK and guaranteed unpersist cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
August 18, 2026 20:37
View session
Contributor
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/LightGBMUtils.scala:18
- LightGBM boolean parameters also accept "on" (e.g., force_col_wise=on / deterministic=on). isEnabledParameterValue currently treats only 1/+1/true/yes as enabled, which can trigger incorrect histogram warnings (or suppress deterministic warnings) when users use the valid "on" syntax.
private val DeviceParamNames = Set("device", "device_type")
private val TrueValues = Set("1", "+1", "true", "yes")
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
## Summary Recognize LightGBM boolean value `on`, cover it in parser regression tests, and clarify that forced-histogram warnings apply only to effective CPU training. ## Prompting Intent Address the new current-head Copilot review threads and suppressed finding on PR microsoft#2663 without broadening product behavior: match accepted LightGBM boolean syntax and align user documentation with the CPU-gated implementation. ## Linked Sources - Pull request: microsoft#2663 - Boolean syntax review: microsoft#2663 (comment) - Documentation review: microsoft#2663 (comment) - Suppressed current-head finding in Copilot review submitted 2026-08-18T20:40:58Z ## Rationale Extending the shared enabled-value set keeps deterministic and forced-histogram parsing consistent for `on` without changing parameter serialization. Updating the existing documentation sentence avoids implying that CPU-only histogram guidance applies to GPU or CUDA training. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
Addressed the current-head suppressed finding in 3e62306: LightGBM boolean value |
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
August 18, 2026 21:12
View session
## Summary Separate the cheap nondeterministic-plan warning fixture from the full LightGBM training fixture and remove redundant source caching and count actions from the Parquet regression. ## Prompting Intent Perform a maintainability and overengineering audit of PR microsoft#2663, retaining exact public-estimator proof while avoiding unnecessary materialization, flaky nondeterministic training input, hot-path changes, or altered estimator semantics. ## Linked Sources - Pull request: microsoft#2663 - GitHub issue: microsoft#2394 - Snapshot review: microsoft#2663 (comment) ## Rationale The warning test only needs a nondeterministic logical plan, so it now uses a ten-row range instead of fitting StringIndexer and assembling the full training fixture. The parity test uses deterministic source data and materializes only its two canonical comparison layouts; the required row collection both proves equality and fills those caches, eliminating a third cache and three redundant count actions without weakening repeated model, prediction, metadata, or AUC checks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Rana Singh (ranadeepsingh)
August 18, 2026 22:07
View session
Collaborator
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
deterministic=truetraining uses nondeterministic Spark lineageonLightGBMClassifierParquet regression covering exact rows, metadata, vector representation, weights, init scores, repeated native model dumps, predictions, and AUCFindings
The issue's dramatic AUC difference was not reproducible as a Parquet or SynapseML serialization defect. Under LightGBM's documented deterministic contract, exact model comparison succeeds when the same stable snapshot and physical row/partition layout are used.
The report's unseeded Spark random ordering is a nondeterministic query plan, and write, fit, and evaluation are separate actions. A Parquet read can also change physical row/partition order without changing serialized values or metadata. The regression therefore keeps nondeterministic lineage solely in a cheap plan-only warning fixture, verifies exact rows/metadata/vector representation, then canonicalizes and materializes both training layouts only inside the test before exact model/prediction comparison. No product sort, repartition, caching, estimator-default, or serialization behavior changed.
A near-perfect training AUC with stable validation AUC can additionally reflect overfit rather than data corruption.
Maintainability audit
try/finallyguarantees cleanup; redundant source caching and three count actions were removed.Review fixes
getAndIncrementPort()instead of fixed port13940.onis recognized and covered by parser regression tests.onfinding was addressed and recorded in a PR comment.Validation
Spark 3.5 / Scala 2.12 / JDK 11
LightGBMParquetReproducibilitySuite: 2/2 passed on head79f16fdb2clightgbm/compileandlightgbm/Test/compilesbt codegenblack==22.3.0 --check --extend-exclude 'docs/' .Spark 4.1 / Scala 2.13 / JDK 17 replay
LightGBMParquetReproducibilitySuite: 2/2 passed on replay of head79f16fdb2clightgbm/compileandlightgbm/Test/compilesbt codegenblack==22.3.0 --check --extend-exclude 'docs/' .Azure validation is intentionally not retriggered here; build 231708782 was canceled before useful validation, and the parent will rerun serialized.
Refs #2394