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.
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
Luca/fix raw predict issue 635 #637
Luca/fix raw predict issue 635 #637
Changes from 3 commits
ce83edc
91f282b
1a444f0
4c31d3e
e7877bd
47053e0
d173f94
11848c3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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.
are you sure Provided.spark isn't enough? it seems like that for the other parity tests?
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.
So I actually meant to get your input on these dependency changes.
Both
Provided.spark
orTest.spark
achieve the same thing here because I am using spark things only in tests.I think that putting those dependencies with a
test
scope makes the build faster, since the dependencies are only brought in when running tests, not when building. I read online:so I assume a test scope is slightly lighter than provided? Again, the xgboost-runtime code has no dependencies on spark stuff, but its tests do.
Another option would be to avoid spark things in tests but I found spark has the best libsvm reader that lets me do
val dataFrame = spark.read.format("libsvm").load(this.getClass.getClassLoader.getResource(filePath).getFile)
and I can use the Dataframe -> to mleapFrame utils.
Let me know what you think!
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.
That makes sense, thank you!
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.
where do we use sparkTestkit?
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.
If I don't bring that in, I cannot use:
import org.apache.spark.sql.mleap.TypeConverters
Do you think it is ok to use that in tests? (%"test" should not be included in the build)
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.
I think that comes from mleap-spark-base, but spark-testkit should be ok too.