-
Notifications
You must be signed in to change notification settings - Fork 242
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
com.nvidia.spark.rapids.ColumnarRdd not exposed to user for XGBoost #3607
Conversation
build |
sql-plugin/src/main/scala/com/nvidia/spark/rapids/ColumnarRdd.scala
Outdated
Show resolved
Hide resolved
I understand we need to solve CNF in general since it's publicly documented in the doc. However, for the case of #3601 it can be argued that the bug fix can be deferred to the client application to use thread context class loader that should have already been set up by the time they call it instead of relying on the AppLauncher classloader: https://github.com/NVIDIA/spark-xgboost/blob/623b0cf172ddaabbf86d238d355294e92a48e321/jvm-packages/xgboost4j-spark/src/main/scala/ml/dmlc/xgboost4j/scala/spark/rapids/GpuUtils.scala#L37 |
build |
@@ -289,6 +289,12 @@ object ShimLoader extends Logging { | |||
shimProviderClass = classname | |||
} | |||
|
|||
def newClassOf(className: String): Class[_] = { |
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.
nit: consider loadClass
as the name
nit be made resusable for newInstanceOf
@@ -343,4 +349,8 @@ object ShimLoader extends Logging { | |||
def newInternalExclusiveModeGpuDiscoveryPlugin(): ResourceDiscoveryPlugin = { | |||
newInstanceOf("com.nvidia.spark.rapids.InternalExclusiveModeGpuDiscoveryPlugin") | |||
} | |||
|
|||
def newColumnarRDDClass(): Class[_] = { |
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.
nit: I'd name newXyz for methds returning objects and loadXyz for classes
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.
LGTM
fixes #3601
we were not exposing ColumnarRDD as an unshimmed class. This fixes that. I tested on xgboost with the python notebook and verified it works now.
This is a bit different since its an object and not a real class we instantiate, so if we have better ways to do this let me know, or we can followup as I'm sure we will have more things that need things like this.