Skip to content

Conversation

@arkq
Copy link
Contributor

@arkq arkq commented Nov 12, 2025

This PR adds dedicated API function nnfw_get_last_error_message() for retrieving last error message in case when other functions return status code other than NNFW_STATUS_NO_ERROR.

It also does small refactoring in order to properly set last error message in all API calls which includes:

  • Refactoring of the loadModel() helper so it will not print to std:cerr
  • Refactoring Session::deprecated function to be non-static to allow setting last error message in it
  • Moving getTensorIndexImpl() helper to be a private member of Session to allow setting last error message in it
  • Testing deprecated APIs with a correct session instance instead of NULL

Also this PR adds small check in the ValidationTestSessionCreated.neg_load_session_2 test case to verify nnfw_get_last_error_message() API correctness.

This PR is fix for one of the issues diagnosed in #16102 (comment)

ONE-DCO-1.0-Signed-off-by: Arkadiusz Bokowy [email protected]

@arkq
Copy link
Contributor Author

arkq commented Nov 18, 2025

Small info how this change will be used.

If approved, it will be possible to get proper error message string in the python binding as follows:

--- a/runtime/onert/api/python/src/wrapper/nnfw_api_wrapper.cc
+++ b/runtime/onert/api/python/src/wrapper/nnfw_api_wrapper.cc
@@ -24,27 +24,26 @@ namespace onert::api::python
 
 namespace py = pybind11;
 
-void ensure_status(NNFW_STATUS status)
+void NNFW_SESSION::ensure_status(NNFW_STATUS status)
 {
   switch (status)
   {
     case NNFW_STATUS::NNFW_STATUS_NO_ERROR:
       return;
     case NNFW_STATUS::NNFW_STATUS_ERROR:
-      throw NnfwError("NNFW_STATUS_ERROR");
+      throw NnfwError(nnfw_get_last_error_message(session));
     case NNFW_STATUS::NNFW_STATUS_UNEXPECTED_NULL:
-      throw NnfwUnexpectedNullError("NNFW_STATUS_UNEXPECTED_NULL");
+      throw NnfwUnexpectedNullError(nnfw_get_last_error_message(session));
     case NNFW_STATUS::NNFW_STATUS_INVALID_STATE:
-      throw NnfwInvalidStateError("NNFW_STATUS_INVALID_STATE");
+      throw NnfwInvalidStateError(nnfw_get_last_error_message(session));
     case NNFW_STATUS::NNFW_STATUS_OUT_OF_MEMORY:
-      throw NnfwOutOfMemoryError("NNFW_STATUS_OUT_OF_MEMORY");
+      throw NnfwOutOfMemoryError(nnfw_get_last_error_message(session));
     case NNFW_STATUS::NNFW_STATUS_INSUFFICIENT_OUTPUT_SIZE:
-      throw NnfwInsufficientOutputError("NNFW_STATUS_INSUFFICIENT_OUTPUT_SIZE");
+      throw NnfwInsufficientOutputError(nnfw_get_last_error_message(session));
     case NNFW_STATUS::NNFW_STATUS_DEPRECATED_API:
-      throw NnfwDeprecatedApiError("NNFW_STATUS_DEPRECATED_API");
-    default:
-      throw NnfwError("NNFW_UNKNOWN_ERROR");
+      throw NnfwDeprecatedApiError(nnfw_get_last_error_message(session));
   }
+  throw NnfwError(nnfw_get_last_error_message(session));
 }
 
 NNFW_LAYOUT getLayout(const char *layout)

@arkq arkq requested a review from Copilot November 25, 2025 08:38
Copilot finished reviewing on behalf of arkq November 25, 2025 08:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new API function nnfw_get_last_error_message() to retrieve error messages from failed API calls, replacing the previous approach of writing errors to std::cerr. The implementation includes comprehensive refactoring to set appropriate error messages in all API functions using a new _last_error_message member variable in the Session class.

  • Adds nnfw_get_last_error_message() API function for retrieving last error message
  • Refactors error handling throughout Session methods to use setLastErrorMessage() instead of std::cerr
  • Converts deprecated API functions from static to instance methods to enable error message tracking

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
runtime/onert/api/nnfw/include/nnfw.h Adds public API declaration for nnfw_get_last_error_message()
runtime/onert/api/nnfw/src/APIImpl.cc Implements nnfw_get_last_error_message() and updates deprecated API wrappers to use instance methods
runtime/onert/api/nnfw/src/Session.h Adds _last_error_message member, get_last_error_message(), setLastErrorMessage() methods, and moves getTensorIndexImpl() to private
runtime/onert/api/nnfw/src/Session.cc Refactors all API methods to call setLastErrorMessage() for error conditions and removes std::cerr writes; refactors loadModel() helper to throw exceptions
runtime/tests/nnfw_api/src/NNPackageTests/SessionCreated.test.cc Adds test for error message retrieval and moves deprecated API tests from SingleSession
runtime/tests/nnfw_api/src/NNPackageTests/SingleSession.test.cc Removes deprecated API tests (moved to SessionCreated)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

arkq added 5 commits November 26, 2025 12:42
This commit adds dedicated API function nnfw_get_last_error_message()
for retrieving last error message in case when other functions return
status code other than NNFW_STATUS_NO_ERROR.

ONE-DCO-1.0-Signed-off-by: Arkadiusz Bokowy <[email protected]>
@arkq arkq force-pushed the session-last-error-message branch from 5659a81 to f63a698 Compare November 26, 2025 11:57
@arkq
Copy link
Contributor Author

arkq commented Nov 26, 2025

@hseok-oh Could you please take a look as this and let me know what do you think? If you like such approach (new API for getting the last error message) I can split that PR into smaller PRs (if you prefer very narrow PRs in terms of scope of changes). Basically, I will submit every commit from this draft as a separate PR.

There is one thing that you should look at, though. The signature of this new API is as follows: nnfw_get_last_error_message(nnfw_session *session). There are some pros and cons with such approach:

pros:

  • every session has its own error message, so one can simultaneously run more than one session and the error message will not be lost
  • in multi threaded application, the error message can be retrieved on a different thread

cons:

  • since we need valid session pointer, this function can not get the error message in case of the nnfw_create_session() failure

Other approach is to hold the error message in the static variable (preferably in the thread local variable) just like the errno value. However, such nnfw_get_last_error_message(void) will be different than the rest of the API calls (every function takes the session pointer as an argument). Also, in multi session scenario on will have to get the error just after every API call. There are also other possibilities, like extending variety of error values, so every single failure will have its own error value (the way openssl does it). Or we can add new API for session creation, something like nnfw_create_session_with_error_message(nnfw_session **session, const char **errmsg) where user can optionally pass an address to a pointer where the static error message will be stored. Anyway, there are only two viable reasons why nnfw_get_last_error_message might fail: 1) passed NULL as session; 2) memory allocation failure. So, the status code should be good enough, just as it is right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant