Skip to content
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

Get error status and message without stderr #63

Closed
LoganDark opened this issue May 17, 2023 · 8 comments · Fixed by #68
Closed

Get error status and message without stderr #63

LoganDark opened this issue May 17, 2023 · 8 comments · Fixed by #68
Labels
good first issue Good for newcomers

Comments

@LoganDark
Copy link
Contributor

Could some GetLastError-like function be added, with error messages or error codes, so it's possible to get error messages without redirecting stderr?

@saharNooby
Copy link
Collaborator

It can be added easily, but I myself have no plans of adding it.

@saharNooby saharNooby added the good first issue Good for newcomers label May 19, 2023
@saharNooby saharNooby changed the title Get error message without stderr Get error status and message without stderr May 19, 2023
@LoganDark
Copy link
Contributor Author

I can pull request a non-breaking change (I.e. existing API consumers will continue to work and print to stderr)

@saharNooby
Copy link
Collaborator

@LoganDark If you mean "on error, functions would still print to stderr; but will also set an error message that can be obatined programmatically with a new function like rwkv_get_last_error", then go ahead :)

@LoganDark
Copy link
Contributor Author

LoganDark commented May 21, 2023

@LoganDark If you mean "on error, functions would still print to stderr; but will also set an error message that can be obatined programmatically with a new function like rwkv_get_last_error", then go ahead :)

I mean I will add a new function that sets (or returns) an error code instead of printing to stderr. the existing function would then call that function internally, and then print the error message to stderr if there is one

@saharNooby
Copy link
Collaborator

@LoganDark Sounds good too, but is it possible to implement it in a way so Python wrapper can still work with older libraries? Users ofter don't recompile/redownload when C API changes, and get broken inference.

Probably Pytnon wrapper can continue to use stderr functions, that'll work.

@LoganDark
Copy link
Contributor Author

LoganDark commented May 22, 2023

@LoganDark Sounds good too, but is it possible to implement it in a way so Python wrapper can still work with older libraries? Users ofter don't recompile/redownload when C API changes, and get broken inference.

I'm not going to be removing any functions, only adding new functions. the existing functions will be kept for backwards compatibility, and will use the new functions internally, but old libraries will not need to know or care

@LoganDark
Copy link
Contributor Author

LoganDark commented May 22, 2023

it looks like this is a bigger task than I thought. this library has very minimal error recovery and seems to just abort the process if anything goes too wrong. maybe the best solution is just to spawn a subprocess and monitor stderr that way. it will be difficult to make this backwards compatible

going to work on it anyway

LoganDark added a commit to LoganDark/rwkv.cpp that referenced this issue May 23, 2023
Fixes RWKV#63

This allows retrieving errors from the library without having to
pipe stderr. Also it was annoying that rwkv.cpp assumed control of
the caller process by doing things like calling abort() when it
shouldn't, so I also fixed that.

The basic way this works is:

1. by default, not much is different, except more errors are caught,
   and rwkv.cpp should never abort the process or throw a C++
   exception.

2. the difference comes when you call rwkv_set_print_errors
   (working title):

   1. errors will no longer be printed to stderr automatically
   2. errors will be assigned to a thread-local variable (during
      init/quantization) or a context-local variable (during eval)
   3. the last error can be retrieved using rwkv_get_last_error

I also overhauled the assert macros so more error cases are
handled:

- the file is now closed if rwkv_init_from_file exits early
- the ggml context is freed if rwkv_init_from_file exits early
- if parameters cannot be found an error will be set about it

I also made some optimizations:

- just use fstat instead of opening the file twice
- deduplicated some code / removed edge cases that do not exist
- switched to ggml inplace operations where they exist

test_tiny_rwkv.c seems to run perfectly fine.

The built DLL is perfectly backwards compatible with existing API
consumers like the python library, because it does not remove or
change any functions, only adds some optional ones.

The sad thing is that this will break every PR because the error
handling in this library was terrible and needed to be totally
redone. But I think it is worth it.
LoganDark added a commit to LoganDark/rwkv.cpp that referenced this issue May 23, 2023
Fixes RWKV#63

This allows retrieving errors from the library without having to
pipe stderr. Also it was annoying that rwkv.cpp assumed control of
the caller process by doing things like calling abort() when it
shouldn't, so I also fixed that.

The basic way this works is:

1. by default, not much is different, except more errors are caught,
   and rwkv.cpp should never abort the process or throw a C++
   exception.

2. the difference comes when you call rwkv_set_print_errors
   (working title):

   1. errors will no longer be printed to stderr automatically
   2. errors will be assigned to a thread-local variable (during
      init/quantization) or a context-local variable (during eval)
   3. the last error can be retrieved using rwkv_get_last_error

I also overhauled the assert macros so more error cases are
handled:

- the file is now closed if rwkv_init_from_file exits early
- the ggml context is freed if rwkv_init_from_file exits early
- if parameters cannot be found an error will be set about it

I also made some optimizations:

- just use fstat instead of opening the file twice
- deduplicated some code / removed edge cases that do not exist
- switched to ggml inplace operations where they exist

test_tiny_rwkv.c seems to run perfectly fine. The Python scripts
also.

The built DLL is perfectly backwards compatible with existing API
consumers like the python library, because it does not remove or
change any functions, only adds some optional ones.

The sad thing is that this will break every PR because the error
handling in this library was terrible and needed to be totally
redone. But I think it is worth it.
@LoganDark
Copy link
Contributor Author

ok, I opened a huge PR. the library architecture was not too bad, but I did a ton of internal refactoring. it took about 8 hours

LoganDark added a commit to LoganDark/rwkv.cpp that referenced this issue May 23, 2023
Fixes RWKV#63

This allows retrieving errors from the library without having to
pipe stderr. Also it was annoying that rwkv.cpp assumed control of
the caller process by doing things like calling abort() when it
shouldn't, so I also fixed that.

The basic way this works is:

1. by default, not much is different, except more errors are caught,
   and rwkv.cpp should never abort the process or throw a C++
   exception.

2. the difference comes when you call rwkv_set_print_errors
   (working title):

   1. errors will no longer be printed to stderr automatically
   2. errors will be assigned to a thread-local variable (during
      init/quantization) or a context-local variable (during eval)
   3. the last error can be retrieved using rwkv_get_last_error

I also overhauled the assert macros so more error cases are
handled:

- the file is now closed if rwkv_init_from_file exits early
- the ggml context is freed if rwkv_init_from_file exits early
- if parameters cannot be found an error will be set about it

I also made some optimizations:

- just use fstat instead of opening the file twice
- deduplicated some code / removed edge cases that do not exist
- switched to ggml inplace operations where they exist

test_tiny_rwkv.c seems to run perfectly fine. The Python scripts
also.

The built DLL is perfectly backwards compatible with existing API
consumers like the python library, because it does not remove or
change any functions, only adds some optional ones.

The sad thing is that this will break every PR because the error
handling in this library was terrible and needed to be totally
redone. But I think it is worth it.
saharNooby added a commit that referenced this issue May 24, 2023
* Add rwkv_set_print_errors and rwkv_get_last_error

Fixes #63

This allows retrieving errors from the library without having to
pipe stderr. Also it was annoying that rwkv.cpp assumed control of
the caller process by doing things like calling abort() when it
shouldn't, so I also fixed that.

The basic way this works is:

1. by default, not much is different, except more errors are caught,
   and rwkv.cpp should never abort the process or throw a C++
   exception.

2. the difference comes when you call rwkv_set_print_errors
   (working title):

   1. errors will no longer be printed to stderr automatically
   2. errors will be assigned to a thread-local variable (during
      init/quantization) or a context-local variable (during eval)
   3. the last error can be retrieved using rwkv_get_last_error

I also overhauled the assert macros so more error cases are
handled:

- the file is now closed if rwkv_init_from_file exits early
- the ggml context is freed if rwkv_init_from_file exits early
- if parameters cannot be found an error will be set about it

I also made some optimizations:

- just use fstat instead of opening the file twice
- deduplicated some code / removed edge cases that do not exist
- switched to ggml inplace operations where they exist

test_tiny_rwkv.c seems to run perfectly fine. The Python scripts
also.

The built DLL is perfectly backwards compatible with existing API
consumers like the python library, because it does not remove or
change any functions, only adds some optional ones.

The sad thing is that this will break every PR because the error
handling in this library was terrible and needed to be totally
redone. But I think it is worth it.

* Fix typo

Co-authored-by: Alex <saharNooby@users.noreply.github.com>

* Visual Studio lied and _fileno is incorrect

* Fix trailing comma in assert macros

This was an accident left over from something that didn't pan out,
some compilers do not like when function arguments have a trailing
comma.

* Include header file for fstat

* Remove uses of std::make_unique

* Fix width of format string argument on all platforms

* Use C free for smart pointers

* Revert "Use C free for smart pointers" and try nothrow

* Initialize cgraph to zero

* Fix ggml_cgraph initialization

* Zero-initialize allocations

---------

Co-authored-by: Alex <saharNooby@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants