-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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-32907][ML] adaptively blockify instances - revert blockify gmm #29782
Conversation
Test build #128806 has finished for PR 29782 at commit
|
Test build #128807 has finished for PR 29782 at commit
|
@@ -432,13 +429,6 @@ def setAggregationDepth(self, value): | |||
""" | |||
return self._set(aggregationDepth=value) | |||
|
|||
@since("3.1.0") | |||
def setBlockSize(self, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zero323 FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems OK if this is just reverting something that went into 3.1.
Feel free to merge |
Jenkins retest this |
Do we need to revert the change https://github.com/apache/spark/pull/27473/files#r493100573 and https://github.com/apache/spark/pull/27473/files#r493100907 |
@WeichenXu123 Good catch! Thanks |
Test build #129002 has finished for PR 29782 at commit
|
Merged to master. Thanks all for reviewing! |
What changes were proposed in this pull request?
revert blockify gmm
Why are the changes needed?
@WeichenXu123 and I thought we should use memory size instead of number of rows to blockify instance; then if a buffer's size is large and determined by number of rows, we should discard it.
In GMM, we found that the pre-allocated memory maybe too large and should be discarded:
We had some offline discuss and thought it is better to revert blockify GMM.
Does this PR introduce any user-facing change?
blockSize added in master branch will be removed
How was this patch tested?
existing testsuites