lanl / ports-of-call

Performance Portability Utilities
https://lanl.github.io/ports-of-call/
BSD 3-Clause "New" or "Revised" License
2 stars 1 forks source link

add portable error handling #26

Closed Yurlungur closed 1 year ago

Yurlungur commented 1 year ago

PR Summary

In the Vinet PR by @aematts in singularity-eos some issues with the current error handling in singularity-eos were noted. I believe a performance-portable error handling treatment should be developed and properly lives in ports-of-call. I introduce it here. In particular, I define several flavors of macro:

All the above are no-ops by default, but are enabled for debug builds. I also provide PORTABLE_ALWAYS_* macros which always work.

The idea here is by default error handling should be silent unless you're debugging. But if the developer knows they want a message or a failure, they can insert one. For the Vinet PR, initialization errors should probably be handled with PORTABLE_THROW_IFEXCEPT for example.

PR Checklist

Yurlungur commented 1 year ago

But rather than some of these just not working on device, should they instead do nothing? My issue is that if a "portable" implementation doesn't work on device, it begs the question as to whether it's truly portable anymore.

That's a fair point. This was written this way to support exception handling on host, when that's desirable. For example, you can imagine some initialization function that uses C++ std exception logic. Basically I wanted to provide that, also for testing. For example the catch machinery Anne wants use for Vinet catches exceptions.

EDIT: This discussion is happening above in the thread with @jhp-lanl

Yurlungur commented 1 year ago

Yep I'll wait to hear from @chadmeyer and @dholladay00 before merging.

Yurlungur commented 1 year ago

@dholladay00 and @chadmeyer what are your thoughts here?

mauneyc-LANL commented 1 year ago

Some documentation in the code wouldn't hurt, though I leave it to your discretion.

Yurlungur commented 1 year ago

Some documentation in the code wouldn't hurt, though I leave it to your discretion.

I don't mind adding more documentation. I didn't add any here because I didn't see what would be a useful addition in this case. What would you find helpful @mauneyc-LANL ?

mauneyc-LANL commented 1 year ago

Some documentation in the code wouldn't hurt, though I leave it to your discretion.

I don't mind adding more documentation. I didn't add any here because I didn't see what would be a useful addition in this case. What would you find helpful @mauneyc-LANL ?

Really just a bit of comments to help "paragraph" the code a little and make it easier to parse.

/***
Used when conditions are encountered where execution could not complete. 
Takes an `input_message`, `filename`, and `line_no`  describing the error, and 
wraps it into formatted output `output_message`.
***/
PORTABLE_INLINE_FUNCTION
void error_msg(const char *const input_message, const char *const filename,
               int const linenumber, char *output_message) {
  std::sprintf(output_message,
               "### ERROR\n  Message:     %s\n  File:        %s\n  Line number: %i\n",
               input_message, filename, linenumber);
}

(probably something less illiterate)

Yurlungur commented 1 year ago

Some documentation in the code wouldn't hurt, though I leave it to your discretion.

I don't mind adding more documentation. I didn't add any here because I didn't see what would be a useful addition in this case. What would you find helpful @mauneyc-LANL ?

Really just a bit of comments to help "paragraph" the code a little and make it easier to parse.

/***
Used when conditions are encountered where execution could not complete. 
Takes an `input_message`, `filename`, and `line_no`  describing the error, and 
wraps it into formatted output `output_message`.
***/
PORTABLE_INLINE_FUNCTION
void error_msg(const char *const input_message, const char *const filename,
               int const linenumber, char *output_message) {
  std::sprintf(output_message,
               "### ERROR\n  Message:     %s\n  File:        %s\n  Line number: %i\n",
               input_message, filename, linenumber);
}

(probably something less illiterate)

Sure I'll add a little bit.

Yurlungur commented 1 year ago

@mauneyc-LANL take a look now. Does this help? I tried to describe the intent of the design as well... e.g., why there are both macros and functions.

mauneyc-LANL commented 1 year ago

@mauneyc-LANL take a look now. Does this help? I tried to describe the intent of the design as well... e.g., why there are both macros and functions.

looks good

jhp-lanl commented 1 year ago

@Yurlungur is this ready now?

Yurlungur commented 1 year ago

@Yurlungur is this ready now?

as soon as I get OK from @dholladay00 and/or @chadmeyer

Yurlungur commented 1 year ago

@dholladay00 how's it look now?