coreylowman / cudarc

Safe rust wrapper around CUDA toolkit
Apache License 2.0
593 stars 73 forks source link

add additional logging for error handling #265

Closed joshpopelka20 closed 3 months ago

joshpopelka20 commented 3 months ago

Adding some additional logging for the result init. I've added them as println, but that can be updated if you'd only want to run during debugging

EricLBuehler commented 3 months ago

For more info: EricLBuehler/mistral.rs#395.

coreylowman commented 3 months ago

So this method is called on pretty much every time a driver function call is called. I don't think we necessarily want the success printed every time. I scanned the linked issue and it seems a lot of it is trying to identify where the error is coming from & what it is.

It seems like the actual issue is in this comment https://github.com/EricLBuehler/mistral.rs/issues/395#issuecomment-2189994656

which is the unwrap in the Debug impl for DriverError

let err_str = self.error_string().unwrap();

We at least need to handle that result from calling error_string properly, instead of calling unwrap. Likely something like:

match self.error_string() {
    Ok(err_str) => /* current code */,
    Err(driver_error) => // Maybe something about being unable to display information about the error? This indicates some additional error happened when trying to call `error_string`
}

That's one issue, I'm not sure if there are additional issues.

joshpopelka20 commented 3 months ago

Thanks for reviewing this! Yes, the unwrap function was an issue for me. Though, even when I took it out, it wasn't giving me any valuable output (in the linked Github issue, you can see that it was saying "Segmentation Fault"). For my error, I needed the additional logging that's in this PR.

I'll remove the success println. For the error one, should I leave as is? Or, would you prefer, I bring in a new crate for logging like log (https://docs.rs/log/latest/log/)?

coreylowman commented 3 months ago

I'll remove the success println. For the error one, should I leave as is? Or, would you prefer, I bring in a new crate for logging like log

I'd really like to minimize the dependencies personally. This is the first time logging has come up, which leads me to believe the current method has been working so far (certainly open to hearing opinions/discussion if you want to open an issue for it).

With the Debug impl error fixed, you can really add the logging anywhere on the stack since the error is returned with the Result.

If that works for you, we probably don't need these changes

joshpopelka20 commented 3 months ago

The changes you added seems to be sufficient. I'll close this PR. Thanks for looking into this!