You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
featureFractionByNode has no default and its explicit value never reaches LightGBM, so users cannot configure node-level feature sampling through the estimator parameter. This PR fixes the default and native forwarding for classifiers, regressors, and rankers, while preserving sampling configured through native aliases after estimator save/load.
Related Issues/PRs
Part of #789 (feature_fraction_bynode only; other parameters remain outside this PR).
What changes are proposed in this pull request?
Set the default to 1.0 and forward explicit values as native feature_fraction_bynode.
The contributor's two commits are preserved. Additive maintainer follow-up 9cb500d30d fixes a regression exposed by the new default: save/load materializes defaults as explicit parameters, which would otherwise override colsample_bynode and sub_feature_bynode. The existing native-argument parser detects these aliases and prevents a generated canonical value from shadowing them. An explicitly supplied canonical pass-through key still takes native precedence; public signatures and serialized parameter shapes are unchanged.
A second focused follow-up, dc111546aa, addresses the subsequent review finding: canonical names also use exact parsed-name matching, so not_feature_fraction_bynode cannot suppress the typed value and quoted canonical keys do not acquire a generated duplicate. The shared parameter builder is unchanged.
Scala and generated Python wrapper regressions cover defaults, copy/clear, persistence, bulk/streaming training, native effective values, evaluated predictions, and pass-through overrides. The Scala suite lives in lightgbm.split1, selected by the existing lightgbm1 UnitTests leg.
How is this patch tested?
I have written tests (not required for typo or doc fix) and confirmed the proposed feature/bug-fix/change works.
Final upstream CI:Azure build 244078667, triggered with /azp run, succeeded across all 65 jobs. Its merge commit 17d05f4dd56ce6f2afd2d9d57a7bf8fa18b8a04b has final head dc111546aaf2c1b5ee74015ee8d15cc793d0a45f and current master e65143c6df061f6e763cc8f7c7d5a99eaecd3e7e as its verified parents.
Published results confirm 78 parameter tests, 39 common tests (one existing ignored performance test), and 19 LightGBM Python tests, including all three parameter-test methods, passed. The parameter suite and Python job have no skips. The Python job used the pinned Python 3.11.8 and ran sbt "project lightgbm" coverage testPython. All four Databricks GPU notebooks passed as well. GitHub checks are green, and the final-head automated review has no open or suppressed findings.
One unrelated geospatial test/coverage-publication TLS failure required an isolated failed-job retry; the other 64 jobs were not rerun. The retry succeeded and uploaded coverage without source, dependency, or policy changes. Geospatial's original console records 17 passed tests, but its per-test records remain absent from Azure's test-run API; those 17 are console-only evidence, not part of the published-result claims above.
Maintainer follow-ups, final head dc111546aa: all 12 public estimator-persistence regressions fail on the original PR head 7ce5a7a005 at the native sampling assertion and pass with the follow-up. Both native aliases retain their sampling fraction and evaluated predictions after save/load. All six unrelated-name public training regressions fail on 9cb500d30d and pass on the final head. Quoted and unquoted canonical-key controls also pass.
Local checks used WSL, Spark 3.5.0, Scala 2.12.17, JDK 11, and the exact patched sources. SBT ran through the repository's JDK-selection wrapper in a native-filesystem source archive, with set ThisBuild / version := "0.0.0-pr2797-review" to avoid Git version discovery.
Passed on the first follow-up; generated API unchanged by the second
lightgbm/codegen
Passed on the first follow-up; generated API unchanged by the second
black --check lightgbm/src/test/python/synapsemltest/lightgbm/test_feature_fraction_by_node.py passed with Black 22.3.0. All three generated-wrapper unittest methods passed using a local classpath/provenance harness, including both aliases in both transfer modes; the harness is session-local, not a repository test entry point. Local Python was 3.12.3, not the manifest-pinned 3.11.8; the completed upstream run above supplies pinned-runtime evidence.
Contributor's earlier evidence (before the maintainer follow-up):original full validation and CI-registration follow-up passed in Linux x86-64 Docker with Spark 3.5.0, Python 3.11.8, and Java 11. The registration follow-up reports 104 Scala tests (one coverage guard, 57 parameter cases, 39 common cases, seven core cases), with one existing ignored test. Original generated-wrapper validation reports 33 Python tests and no skips. These results belong to the contributor's earlier revisions, not the new maintainer commit.
The original upstream baseline fails the forwarding/default regressions as expected. The earlier unregistered Scala suite fails PipelineTestCoverageSuite; moving it into lightgbm.split1 passes that guard.
Does this PR change any dependencies?
No. You can skip this section.
Yes. Make sure the dependencies are resolved correctly, and list changes here.
Does this PR add a new feature? If so, have you added samples on website?
No. You can skip this section.
Yes. Make sure you have added samples following below steps.
Find the corresponding markdown file for your new feature in website/docs/documentation folder.
Make sure you choose the correct class estimators/transformers and namespace.
Follow the pattern in markdown file and add another section for your new API, including pyspark, scala (and .NET potentially) samples.
Make sure the DocTable points to correct API link.
Navigate to website folder, and run npm start to make sure the website renders correctly.
Don't forget to add <!--pytest-codeblocks:cont--> before each python code blocks to enable auto-tests for python samples.
Make sure the WebsiteSamplesTests job pass in the pipeline.
Hey Lenin Mookiah (@leninworld) 👋!
Thank you so much for contributing to our repository 🙌.
Someone from SynapseML Team will be reviewing this pull request soon.
We use semantic commit messages to streamline the release process.
Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix.
This helps us to create release messages and credit you for your hard work!
Examples of commit messages with semantic prefixes:
fix: Fix LightGBM crashes with empty partitions
feat: Make HTTP on Spark back-offs configurable
docs: Update Spark Serving usage
build: Add codecov support
perf: improve LightGBM memory usage
refactor: make python code generation rely on classes
style: Remove nulls from CNTKModel
test: Add test coverage for CNTKModel
To test your commit locally, please follow our guild on building from source.
Check out the developer guide for additional guidance on testing your change.
Repairs the existing LightGBM featureFractionByNode parameter for classifiers, regressors, and rankers, addressing the node-sampling portion of issue #789.
Changes:
Corrects the parameter’s default to 1.0.
Forwards explicit values as native feature_fraction_bynode.
Adds Scala and Python wrapper regressions for parameter handling, training, persistence, and override behavior.
## Summary
Prevent the featureFractionByNode default materialized by estimator
persistence from overriding colsample_bynode or sub_feature_bynode.
Keep the contributor's default and native forwarding fix, while respecting
pass-through aliases and explicitly supplied canonical native keys.
## Prompting Intent
Patch the independently reproduced regression on the existing contributor
PR, preserve the author's commits, and prove public training and persistence
behavior before publishing an additive maintainer follow-up.
## Linked Sources
- Contribution: microsoft#2797
- Parameter request: microsoft#789
- Reviewed head: microsoft@7ce5a7a
- Initial validation: https://github.com/leninworld/SynapseML/actions/runs/41214304144
- CI-registration validation: https://github.com/leninworld/SynapseML/actions/runs/41219442947
## Rationale
Reuse the existing native-argument parser rather than changing the shared
serializer, public JVM signatures, or serialized parameter shapes. Omit only
this generated canonical argument when a native alias is already supplied;
LightGBM still resolves an explicitly supplied canonical key normally.
Replace the tests expecting alias shadowing with native-value and evaluated
prediction preservation checks. Cover typed overrides, complete alias names,
canonical-key controls, all learners and both transfer modes in Scala, plus
generated Python estimator persistence.
The 12 public persistence regressions fail on the original PR head and pass
with this change. All 66 parameter tests and 39 existing common tests pass
(one existing ignored test). Compilation, main/test Scala style, Black
22.3.0, core/LightGBM codegen, and all three generated-wrapper test methods
pass locally. JVM/Python provenance is the patched checkout. Local runtime:
Spark 3.5.0, JDK 11 and Python 3.12.3; upstream pinned-runtime CI is pending.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eef3b166-4731-41dc-a67b-83619ec8e85e
Maintainer follow-up blocked by branch permissions
Lenin Mookiah (@leninworld), the new default exposes an estimator-persistence regression: colsample_bynode=0.7 and sub_feature_bynode=0.7 train with 0.7 before save/load but 1.0 afterward. The previous target preserves 0.7. The canonical native-key control remains unchanged.
I prepared a small additive fix that keeps your default/forwarding change and uses the existing native-argument parser to avoid shadowing pass-through aliases. It also replaces the reloaded-default expectations with public fit/transform preservation checks. All 12 updated persistence regressions fail on the current PR head for the native-value mismatch and pass with the correction; 66 parameter cases and 39 common tests pass locally, with one pre-existing ignored test. Generated-wrapper tests, compilation, style and codegen also pass. Local runtime was Spark 3.5.0, JDK 11 and Python 3.12.3; required pinned-runtime CI has not run for this follow-up.
GitHub rejected the fast-forward update with permission denied; this PR currently has maintainer edits disabled. Please enable Allow edits from maintainers if you want me to apply the validated follow-up, or let us know if you prefer to make the change yourself. No contributor commits were rewritten, and no code was pushed. This is a permissions blocker, not a merge-readiness or sign-off request.
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The canonical parameter check still matches unrelated key substrings, which can suppress an explicitly configured feature-sampling value and needs exact-name handling with regression coverage.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Canonical feature fraction can be suppressed by an unrelated pass-through key
The canonical key still uses a substring check: setFeatureFractionByNode(0.4).setPassThroughArgs("not_feature_fraction_bynode=0.7") emits no feature_fraction_bynode token because appendParamValueIfNotThere matches the suffix of the unrelated key. The explicit sampling value is therefore lost, despite the exact-name handling added for aliases. Check the parsed canonical name before appending too, and extend the existing negative-name regression to cover the canonical key.
## Summary
Prevent unrelated pass-through names from suppressing an explicitly set
featureFractionByNode, and avoid generating duplicate quoted canonical keys.
## Prompting Intent
Complete the contributor-PR follow-up using the established process: preserve
the contributor's commits, address current-head review findings, validate the
public behavior, and obtain CI evidence before requesting contributor sign-off.
## Linked Sources
- Contribution: microsoft#2797
- Parameter request: microsoft#789
- Suppressed review finding: microsoft#2797 (review)
- Earlier alias repair: microsoft@9cb500d
## Rationale
Use the existing native parser for the canonical name as well as its aliases,
then append the already-checked value directly. This avoids the shared
parameter builder's substring matching without changing unrelated parameters,
public APIs, or persistence shapes.
All six new unrelated-name public training cases fail on the previous head
at the native sampling assertion. The corrected implementation passes all
78 parameter cases and 39 common cases, with one pre-existing ignored test.
Main/test compilation and Scala style pass, as do the three generated-wrapper
Python methods. The tests also cover quoted canonical keys and model
prediction preservation in bulk and streaming for all three learners.
Local validation used Spark 3.5.0, JDK 11, and Python 3.12.3. Full upstream
pinned-runtime CI must run again for this commit.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eef3b166-4731-41dc-a67b-83619ec8e85e
Addressed the suppressed canonical-name finding in additive commit dc111546aa. Canonical and alias names now share exact-name parsing, without changing the shared parameter builder. All six unrelated-name public training regressions fail on the previous head and pass with this correction. The final local run passes 78 parameter tests plus 39 common tests (one existing ignored), compilation/style, and all three generated-wrapper Python methods. Quoted canonical controls and persistence predictions pass as well. Full CI and automated review must complete for this new head; the earlier build is not final-head evidence.
Thanks Lenin Mookiah (@leninworld) for fixing the featureFractionByNode default and forwarding, and for adding the regression coverage! This makes the existing node-sampling parameter work for classifiers, regressors, and rankers.
I preserved both of your commits and added two focused follow-ups:
9cb500d30d keeps your default/forwarding repair while preventing reloaded defaults from overriding colsample_bynode and sub_feature_bynode. The tests now check native sampling and evaluated predictions across estimator save/load.
dc111546aa addresses the subsequent canonical-name review finding: unrelated names cannot suppress the typed value, and quoted canonical keys do not gain a generated duplicate. The shared parameter builder and public serialization shapes are unchanged.
All 12 alias-persistence regressions fail on the original PR head and pass with the fix. The six unrelated-name training regressions likewise fail before the second follow-up and pass afterward.
Azure build 244078667, triggered with /azp run, passed all 65 jobs for final head dc111546aaf2c1b5ee74015ee8d15cc793d0a45f through its verified merge with current master. Published results confirm 78 parameter cases, 39 common cases, and all 19 LightGBM Python tests passed, including the three parameter-test methods on pinned Python 3.11.8. Only the existing common performance test is ignored within those suites; the parameter suite and Python job have no skips. GitHub checks are green, and the current-head automated review has no open or suppressed findings.
An unrelated geospatial publication TLS failure required an isolated retry, which succeeded without source or policy changes. Its original console shows 17 passed tests, but the per-test records remain absent from Azure's test-run API; those are console-only evidence, not included in the published counts above.
Please review these follow-ups and sign off if they fit your intent. If they do not fit your vision for the PR, feel free to revert my two commits, or let me know and I will revert them. Contributor sign-off and the required human maintainer approval remain; this comment is not a merge approval. The other parameter requests in #789 remain outside this PR.
Thanks Lenin Mookiah (Lenin Mookiah (@leninworld)) for fixing the featureFractionByNode default and forwarding, and for adding the regression coverage! This makes the existing node-sampling parameter work for classifiers, regressors, and rankers.
I preserved both of your commits and added two focused follow-ups:
9cb500d30d keeps your default/forwarding repair while preventing reloaded defaults from overriding colsample_bynode and sub_feature_bynode. The tests now check native sampling and evaluated predictions across estimator save/load.
dc111546aa addresses the subsequent canonical-name review finding: unrelated names cannot suppress the typed value, and quoted canonical keys do not gain a generated duplicate. The shared parameter builder and public serialization shapes are unchanged.
All 12 alias-persistence regressions fail on the original PR head and pass with the fix. The six unrelated-name training regressions likewise fail before the second follow-up and pass afterward.
Azure build 244078667, triggered with /azp run, passed all 65 jobs for final head dc111546aaf2c1b5ee74015ee8d15cc793d0a45f through its verified merge with current master. Published results confirm 78 parameter cases, 39 common cases, and all 19 LightGBM Python tests passed, including the three parameter-test methods on pinned Python 3.11.8. Only the existing common performance test is ignored within those suites; the parameter suite and Python job have no skips. GitHub checks are green, and the current-head automated review has no open or suppressed findings.
An unrelated geospatial publication TLS failure required an isolated retry, which succeeded without source or policy changes. Its original console shows 17 passed tests, but the per-test records remain absent from Azure's test-run API; those are console-only evidence, not included in the published counts above.
Please review these follow-ups and sign off if they fit your intent. If they do not fit your vision for the PR, feel free to revert my two commits, or let me know and I will revert them. Contributor sign-off and the required human maintainer approval remain; this comment is not a merge approval. The other parameter requests in #789 remain outside this PR.
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
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.
featureFractionByNodehas no default and its explicit value never reaches LightGBM, so users cannot configure node-level feature sampling through the estimator parameter. This PR fixes the default and native forwarding for classifiers, regressors, and rankers, while preserving sampling configured through native aliases after estimator save/load.Related Issues/PRs
Part of #789 (
feature_fraction_bynodeonly; other parameters remain outside this PR).What changes are proposed in this pull request?
Set the default to
1.0and forward explicit values as nativefeature_fraction_bynode.The contributor's two commits are preserved. Additive maintainer follow-up 9cb500d30d fixes a regression exposed by the new default: save/load materializes defaults as explicit parameters, which would otherwise override
colsample_bynodeandsub_feature_bynode. The existing native-argument parser detects these aliases and prevents a generated canonical value from shadowing them. An explicitly supplied canonical pass-through key still takes native precedence; public signatures and serialized parameter shapes are unchanged.A second focused follow-up, dc111546aa, addresses the subsequent review finding: canonical names also use exact parsed-name matching, so
not_feature_fraction_bynodecannot suppress the typed value and quoted canonical keys do not acquire a generated duplicate. The shared parameter builder is unchanged.Scala and generated Python wrapper regressions cover defaults, copy/clear, persistence, bulk/streaming training, native effective values, evaluated predictions, and pass-through overrides. The Scala suite lives in
lightgbm.split1, selected by the existinglightgbm1UnitTests leg.How is this patch tested?
Final upstream CI: Azure build 244078667, triggered with
/azp run, succeeded across all 65 jobs. Its merge commit17d05f4dd56ce6f2afd2d9d57a7bf8fa18b8a04bhas final headdc111546aaf2c1b5ee74015ee8d15cc793d0a45fand current mastere65143c6df061f6e763cc8f7c7d5a99eaecd3e7eas its verified parents.Published results confirm 78 parameter tests, 39 common tests (one existing ignored performance test), and 19 LightGBM Python tests, including all three parameter-test methods, passed. The parameter suite and Python job have no skips. The Python job used the pinned Python 3.11.8 and ran
sbt "project lightgbm" coverage testPython. All four Databricks GPU notebooks passed as well. GitHub checks are green, and the final-head automated review has no open or suppressed findings.One unrelated geospatial test/coverage-publication TLS failure required an isolated failed-job retry; the other 64 jobs were not rerun. The retry succeeded and uploaded coverage without source, dependency, or policy changes. Geospatial's original console records 17 passed tests, but its per-test records remain absent from Azure's test-run API; those 17 are console-only evidence, not part of the published-result claims above.
Maintainer follow-ups, final head
dc111546aa: all 12 public estimator-persistence regressions fail on the original PR head7ce5a7a005at the native sampling assertion and pass with the follow-up. Both native aliases retain their sampling fraction and evaluated predictions after save/load. All six unrelated-name public training regressions fail on9cb500d30dand pass on the final head. Quoted and unquoted canonical-key controls also pass.Local checks used WSL, Spark 3.5.0, Scala 2.12.17, JDK 11, and the exact patched sources. SBT ran through the repository's JDK-selection wrapper in a native-filesystem source archive, with
set ThisBuild / version := "0.0.0-pr2797-review"to avoid Git version discovery.lightgbm/compilelightgbm/Test/compilelightgbm/scalastylelightgbm/Test/scalastylelightgbm/testOnly com.microsoft.azure.synapse.ml.lightgbm.split1.VerifyFeatureFractionByNode com.microsoft.azure.synapse.ml.lightgbm.split1.VerifyLightGBMCommoncore/codegenlightgbm/codegenblack --check lightgbm/src/test/python/synapsemltest/lightgbm/test_feature_fraction_by_node.pypassed with Black 22.3.0. All three generated-wrapper unittest methods passed using a local classpath/provenance harness, including both aliases in both transfer modes; the harness is session-local, not a repository test entry point. Local Python was 3.12.3, not the manifest-pinned 3.11.8; the completed upstream run above supplies pinned-runtime evidence.Contributor's earlier evidence (before the maintainer follow-up): original full validation and CI-registration follow-up passed in Linux x86-64 Docker with Spark 3.5.0, Python 3.11.8, and Java 11. The registration follow-up reports 104 Scala tests (one coverage guard, 57 parameter cases, 39 common cases, seven core cases), with one existing ignored test. Original generated-wrapper validation reports 33 Python tests and no skips. These results belong to the contributor's earlier revisions, not the new maintainer commit.
The original upstream baseline fails the forwarding/default regressions as expected. The earlier unregistered Scala suite fails
PipelineTestCoverageSuite; moving it intolightgbm.split1passes that guard.Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?
website/docs/documentationfolder.Make sure you choose the correct class
estimators/transformersand namespace.DocTablepoints to correct API link.npm startto make sure the website renders correctly.<!--pytest-codeblocks:cont-->before each python code blocks to enable auto-tests for python samples.WebsiteSamplesTestsjob pass in the pipeline.Prepared with AI assistance.