-
Notifications
You must be signed in to change notification settings - Fork 240
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
Throw again after logging that RMM could not intialize #5243
Throw again after logging that RMM could not intialize #5243
Conversation
Signed-off-by: Alessandro Bellina <abellina@nvidia.com>
@@ -301,7 +301,9 @@ object GpuDeviceManager extends Logging { | |||
Rmm.initialize(init, logConf, poolAllocation) | |||
RapidsBufferCatalog.init(conf) | |||
} catch { | |||
case e: Exception => logError("Could not initialize RMM", e) | |||
case e: CudfException => | |||
logError("Could not initialize RMM, exiting!", e) |
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.
This is typically an anti-pattern
https://stackoverflow.com/questions/6639963/why-is-log-and-throw-considered-an-anti-pattern
Is there a reason we need the log statement? Should we just lat the exception continue up?
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.
yes agree, I wanted to add something more specific than the error seen here: #5242 (comment), that's the only reason.
Currently this exception that I am re-throwing gets caught up again in the RapidsExecutorPlugin
where we exit
. Perhaps I should change the message here to say we are exiting. Do you have a preference?
22/04/13 14:39:05 ERROR RapidsExecutorPlugin: Exception in the executor plugin
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 would rather have the code that exits say we are exiting, otherwise there is coupling between the code that exits and here for no good reason.
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.
build |
Signed-off-by: Alessandro Bellina abellina@nvidia.com
Closes: #5242.
This is a (small) proposed diff to make failure to initialize RMM (which throws a
CudfException
) fatal, instead of silently continuing with a default initialized raw cuda memory resource.The executor logs will look like this: