ORNL / ORNL/ReSolve

improve error handling and propagation in resolve

Open
#344 1 comment 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

bug compiler warnings documentation enhancement
Dominant language
C++
Stars
84
Forks
13
PR merge metrics
No merged PRs in 30d

Description

to prevent something like #337 from happening again, i suggest we do a few things to improve error propagation in resolve.

first, we should annotate all functions which return error values with [[nodiscard]]. this will require that we bump the c++ version to 17, but this will greatly improve the ease at which resolve allows its users to write fast, correct code. this is only a stopgap solution, of course, as the current way in which we handle errors leaves a lot to be desired.

right now, the way we handle errors is by returning a simple integer value of indeterminate value (see the transpose implementation using hip, the various stub methods which return 1, etc). there are several issues with this approach. for one, there is no canonical "error code" description, our most useful output channel for the semantics around errors consists of the logging channels. two, these methods don't even necessarily return an "error code". again, see the transpose implementation using the hip backend, which returns a sum of the error codes returned by the rocsparse functions which is semantically meaningless due to these values on their own not just representing the existence of an error but also the kind of error itself.

the issue with all of this is that it doesn't allow the people calling resolve code to consistently handle the error beyond guessing what it is. if we had logged the return value of the transpose function in #337, we would not have known much of anything from that alone, as the function itself does not bail if the buffer sizing function call fails and will sum the return values of the buffer sizing function and the csr to csc function call.

to fix this, i suggest we create our own error structure that abstracts over errors stemming from our own code and those in libraries we depend upon, such as rocm. this could be an enum, but because we need to wrap over cuda, hip, and other libraries, it's probably best to provide a way to extract out the underlying error from this, so an enum wouldn't work. this would allow calling code to properly handle the causes of errors where this is possible. we should return this structure from our own functions and annotate the structure itself with [[nodiscard]] so that just ignoring it results in a warning.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with resolve/matrix/MatrixHandlerHip.cpp around the HIP transpose implementation and resolve/LinSolverIterative.cpp around the stub methods, then read issue #337 for the failure context. Trace how resolve functions return and propagate errors across CUDA, HIP, and other backends. Done means the scope and design support a common error structure, underlying dependency errors, and [[nodiscard]] handling with the proposed C++17 requirement.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend-api-design
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.