Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[spark] Make spark model have the same UID with its estimator #9022

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

WeichenXu123
Copy link
Contributor

Make spark model have the same UID with its estimator.

Because all other pyspark models obey the rule that uses the same UID with estimator, and the UID is used in param copying (e.g. CrossValidator copies param list to estimator, and UID is used for param owner validation), so we need to make the UID correct.

Signed-off-by: Weichen Xu <[email protected]>
@WeichenXu123
Copy link
Contributor Author

CC @wbo4958 @trivialfis Would you mind taking a look ? Thank you!

@WeichenXu123
Copy link
Contributor Author

Can we backport this fix to xgboost 1.7.6 version ?

@trivialfis
Copy link
Member

trivialfis commented Apr 11, 2023

I don't have any plan for 1.7.6 at the moment, is this fix significant? In what scenario this can cause issues?

@WeichenXu123
Copy link
Contributor Author

WeichenXu123 commented Apr 12, 2023

I don't have any plan for 1.7.6 at the moment, is this fix significant? In what scenario this can cause issues?

Only some very edge case (a pyspark pipeline contains xgboost estimator, and crossvalidator tunes the pipeline, and we tunes some params related to xgboost model prediction).

It is not an critical bug so 1.7.6 release is not urgent for . :)

Copy link
Contributor

@wbo4958 wbo4958 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@WeichenXu123
Copy link
Contributor Author

@trivialfis Could you trigger all CI runs and then merge the PR once CI passed? Thankyou!

@WeichenXu123
Copy link
Contributor Author

Reminder: Backport this commit to 1.7 branch :)

@trivialfis trivialfis merged commit 191d0aa into dmlc:master Apr 13, 2023
@trivialfis trivialfis mentioned this pull request Jun 8, 2023
7 tasks
trivialfis pushed a commit to trivialfis/xgboost that referenced this pull request Jun 9, 2023
trivialfis pushed a commit to trivialfis/xgboost that referenced this pull request Jun 10, 2023
trivialfis added a commit that referenced this pull request Jun 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants