[SPARK-51867][ML] Make scala model supporting save / load methods against local filesystem path #50665
Conversation
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
mllib/src/main/scala/org/apache/spark/ml/clustering/KMeans.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
WeichenXu123
changed the title
| * Saves the ML instances to the local file system path. | ||
| */ | ||
| @throws[IOException]("If the input path already exists but overwrite is not enabled.") | ||
| private[spark] def saveToLocal(path: String): Unit = { |
There was a problem hiding this comment.
Instead of introduce a new saveToLocal method, what about adding new class MLLocalWriter extends MLWriter?
Then based on the writer class, we can know whether an algorithm support local read/write
There was a problem hiding this comment.
My concern is changing the base class might influence compatibility,
currently saveToLocal and loadFromLocal are only developing APIs and are only used in Spark connect server for ML cache management.
So that I tend to keep current code.
once we decide to make them as public APIs, we can consider to make a better API / Base class design.
thoughts ?
zhengruifeng
approved these changes
Apr 28, 2025
|
merged to master |
|
It appears that after merging this pr, it caused test failures for Here's how I conducted the local inspection: The reason why this issue wasn't detected during the GitHub Actions of this pr is that the changes in the |
|
I will revert this one first to restore the GitHub Actions workflow for the master. @WeichenXu123 @zhengruifeng |
|
Reopen this pr and we can re-merge it after fixing the above issue |
|
@LuciferYang I updated my PR. it should fix the issue . |
|
Merged into master. Thanks @WeichenXu123 and @zhengruifeng |
| @Since("1.6.0") | ||
| object LogisticRegressionModel extends MLReadable[LogisticRegressionModel] { | ||
| case class Data( |
There was a problem hiding this comment.
I don't know why it fails MLSuite, but we should not make it public.
It seems private[spark] can work. #50760
ericm-db
pushed a commit
to ericm-db/spark
that referenced
this pull request
May 5, 2025
…inst local filesystem path ### What changes were proposed in this pull request? Make scala model supporting save / load methods (deverloper api) against local filesystem path. ### Why are the changes needed? This is required by Spark Connect server model cache management. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unit tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#50665 from WeichenXu123/ml-save-to-local. Authored-by: Weichen Xu <weichen.xu@databricks.com> Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
ericm-db
pushed a commit
to ericm-db/spark
that referenced
this pull request
May 5, 2025
…inst local filesystem path ### What changes were proposed in this pull request? Make scala model supporting save / load methods (deverloper api) against local filesystem path. ### Why are the changes needed? This is required by Spark Connect server model cache management. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unit tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#50665 from WeichenXu123/ml-save-to-local. Authored-by: Weichen Xu <weichen.xu@databricks.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
Kimahriman
pushed a commit
to Kimahriman/spark
that referenced
this pull request
May 13, 2025
…inst local filesystem path ### What changes were proposed in this pull request? Make scala model supporting save / load methods (deverloper api) against local filesystem path. ### Why are the changes needed? This is required by Spark Connect server model cache management. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unit tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#50665 from WeichenXu123/ml-save-to-local. Authored-by: Weichen Xu <weichen.xu@databricks.com> Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
Kimahriman
pushed a commit
to Kimahriman/spark
that referenced
this pull request
May 13, 2025
…inst local filesystem path ### What changes were proposed in this pull request? Make scala model supporting save / load methods (deverloper api) against local filesystem path. ### Why are the changes needed? This is required by Spark Connect server model cache management. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unit tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#50665 from WeichenXu123/ml-save-to-local. Authored-by: Weichen Xu <weichen.xu@databricks.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
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.
What changes were proposed in this pull request?
Make scala model supporting save / load methods (deverloper api) against local filesystem path.
Why are the changes needed?
This is required by Spark Connect server model cache management.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Unit tests.
Was this patch authored or co-authored using generative AI tooling?
No.