Feature/c api #57

ChrisspyB · 2024-12-11T12:01:52Z

No description provided.

codecov-commenter · 2024-12-11T12:09:54Z

Codecov Report

Attention: Patch coverage is 74.90909% with 69 lines in your changes missing coverage. Please review.

Project coverage is 61.11%. Comparing base (963c295) to head (bf9f488).

Files with missing lines	Patch %	Lines
src/metkit/api/metkit_c.cc	64.79%	69 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #57      +/-   ##
===========================================
+ Coverage    60.39%   61.11%   +0.71%     
===========================================
  Files          101      103       +2     
  Lines         6242     6517     +275     
  Branches       585      590       +5     
===========================================
+ Hits          3770     3983     +213     
- Misses        2472     2534      +62

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ChrisspyB · 2024-12-16T09:53:02Z

src/metkit/api/metkit_c.cc

Todo: Ensure we cover all functionality currently in fdb_c so that we can replace it with this.

Looks good so far. The functionality of the pyfdb is the following:

struct fdb_request_t; typedef struct fdb_request_t fdb_request_t; int fdb_new_request(fdb_request_t** req); int fdb_request_add(fdb_request_t* req, const char* param, const char* values[], int numValues); int fdb_expand_request(fdb_request_t* req); int fdb_delete_request(fdb_request_t* req);

Those are all contained in the request section. Do we need anything on top @ChrisspyB?

tbkr

Like the changes. Code and Documentation was extraordinarily clean. I was a bit nitpicky. Bear with me in case some of my suggestions are not compatible with modern C; it has been a while since I programmed in C ;)

src/metkit/api/metkit_c.cc

tbkr · 2024-12-18T14:01:53Z

src/metkit/api/metkit_c.cc

+/// @comment: (maby)
+/// Not sure if there is much value in having a param iterator. We could just return an array of
+/// strings (char**) using metkit_marsrequest_params.
+/// I think we should metkit_paramiterator_t.


Here the comment is lacking some words. Do this need to be addressed before merging?

a todo left for me by me

src/metkit/api/metkit_c.cc

tbkr · 2024-12-18T14:02:30Z

src/metkit/api/metkit_c.cc

+
+}  // extern "C"
+
+static thread_local std::string g_current_error_string;


This is read-only, isn't it? Make it const.

src/metkit/api/metkit_c.cc

tests/test_c_api.cc

tbkr · 2024-12-18T14:32:02Z

tests/test_c_api.cc

+    } while (false)
+
+// Fairly minimal test coverage
+CASE( "metkit_marsrequest" ) {


This test is quite long. Could you break it in individual tests for easier and faster comprehension?

tbkr · 2024-12-18T14:34:08Z

src/metkit/api/metkit_c.h

+ */
+int metkit_marsrequest_add(metkit_marsrequest_t* request, const char* param, const char* values[], int numValues);
+
+/** Add parameter and values to request


Would be nice to have some documentation on why the 1 appears in the function name. Or even better: renaming to function to reflect what is happening.

tbkr · 2024-12-18T14:40:29Z

src/metkit/api/metkit_c.cc

Looks good so far. The functionality of the pyfdb is the following:

struct fdb_request_t; typedef struct fdb_request_t fdb_request_t; int fdb_new_request(fdb_request_t** req); int fdb_request_add(fdb_request_t* req, const char* param, const char* values[], int numValues); int fdb_expand_request(fdb_request_t* req); int fdb_delete_request(fdb_request_t* req);

Those are all contained in the request section. Do we need anything on top @ChrisspyB?

simondsmart · 2024-12-18T16:51:26Z

src/metkit/api/metkit_c.h

+ * @param requests Allocates RequestIterator object
+ * @return int Error code
+ */
+int metkit_parse_marsrequest(const char* str, metkit_requestiterator_t** requests, bool strict = false);


This returns mars requests (plural), but in the function name it is only singular. That isn't cool.

Also, please make sure you run this through a C compiler --- NOT a C++ compiler. Note that C does not support default arguments.

And you will need to include <stdbool.h> if you want to use bool - as bool is not a builtin type in C.

src/metkit/api/metkit_c.h

simondsmart · 2024-12-18T16:56:04Z

src/metkit/api/metkit_c.h

+ * @param params Allocates ParamIterator object for parameter names in request
+ * @return int Error code
+ */
+int metkit_marsrequest_params(const metkit_marsrequest_t* request, metkit_paramiterator_t** params);


It might be easier to return the number of params, and then access them by index? Avoid creating/disposing of the paramiterator?

simondsmart · 2024-12-18T16:57:02Z

src/metkit/api/metkit_c.h

+ * @param numValues number of values for param in request
+ * @return int Error code
+ */
+int metkit_marsrequest_values(const metkit_marsrequest_t* request, const char* param, const char** values[], size_t* numValues);


Does this allocate an array of values in C and then return it? Who is responsible for cleaning up that array of values? Why is this done in a different way to accessing the parameters?

Yes this makes an array of values that the calling code is responsible for cleaning up. Thoughts on that? I was thinking we could do the same with parameters.

Freeing 2d allocations in C is a bit verbose. I suggest either create an opaque type and implement the required query functions or use a public structure that can be returned as handle and has a corresponding free function.

Opaque type

struct marsrequest_values_t; typedef marsrequest_values_t* marsrequest_values_handle; size_t marsrequest_values_size(marsrequest_values_handle h); const char* marsrequest_values_size(size_t idx); void marsrequest_values_free(marsrequest_values_handle h);

Public type

typedef struct { size_t size; const char** values } marsrequest_values; void marsrequest_values_free(marsrequest_values* values);

I think an opaque type would be nice as this could simply be a std::vector<string> on the C++ side.

Ozaq

You need an extra test where you use the API and compile the test with a C compiler

Ozaq · 2024-12-19T13:30:23Z

src/metkit/api/metkit_c.h

+typedef enum metkit_error_values_t
+{
+    METKIT_SUCCESS            = 0, /* Operation succeded. */
+    METKIT_ITERATION_COMPLETE = 1, /* All elements have been returned */


This is not an error and should be handled differently. Right now this mixes error and result states. I would advocate for a strict separation.

src/metkit/api/metkit_c.h

Ozaq · 2024-12-19T13:33:55Z

src/metkit/api/metkit_c.h

+ * @param version Version string
+ * @return int Error code
+ */
+int metkit_version(const char** version);


This call should not be able to fail and therefor just return the version

One convience from every function returning an error code is that it allows us (e.g. on the python layer) to wrap every C function call with a simple error handler that just checks the return value.

Ozaq · 2024-12-19T13:34:20Z

src/metkit/api/metkit_c.h

+ * @param sha1 SHA1 version string
+ * @return int Error code
+ */
+int metkit_vcs_version(const char** sha1);


This call should not be able to fail and therefor just return the sha1

Ozaq · 2024-12-19T13:35:20Z

src/metkit/api/metkit_c.h

+ *
+ * @note This is ONLY required when Main() is NOT initialised, such as loading
+ * the MetKit as shared library in Python.
+ * @return int Error code


This should document the possible error codes returned from this function and under which circumstances the errors are to be expected.

The underlying issue here is that every possible error that can happen when using this API is corralled into metkit_error_values_t. When using such an API you will greatly thank the doc writer for stating the possible error cases, (think man pages for example) because this allows me to simplify error handling. And treat every unexpected EC as fatal error.

If the user is not given any information about the possible errors, he will always be wondering "Could this return a METKIT_ITERATION_COMPLETE?"

Ozaq · 2024-12-19T14:08:20Z

src/metkit/api/metkit_c.h

+ * @param it RequestIterator instance
+ * @return int Error code
+ */
+int metkit_requestiterator_next(metkit_requestiterator_t* it);


I think this would make for a nicer API if we could use it like this:

while((res = metkit_paramiterator_next(it))) { //... }

Ofc this would only work if the iteration cannot create errors. But return a nullptr on end of iteration makes for a more natural api imo.

src/metkit/api/metkit_c.h

Ozaq · 2024-12-19T14:21:16Z

src/metkit/api/metkit_c.h

+ * @param numValues number of values for param in request
+ * @return int Error code
+ */
+int metkit_marsrequest_values(const metkit_marsrequest_t* request, const char* param, const char** values[], size_t* numValues);


Freeing 2d allocations in C is a bit verbose. I suggest either create an opaque type and implement the required query functions or use a public structure that can be returned as handle and has a corresponding free function.

Opaque type

struct marsrequest_values_t; typedef marsrequest_values_t* marsrequest_values_handle; size_t marsrequest_values_size(marsrequest_values_handle h); const char* marsrequest_values_size(size_t idx); void marsrequest_values_free(marsrequest_values_handle h);

Public type

typedef struct { size_t size; const char** values } marsrequest_values; void marsrequest_values_free(marsrequest_values* values);

I think an opaque type would be nice as this could simply be a std::vector<string> on the C++ side.

tests/test_c_api.cc

src/metkit/api/metkit_c.cc

Ozaq · 2024-12-19T14:45:06Z

src/metkit/api/metkit_c.cc

+/// Not sure if there is much value in having a param iterator. We could just return an array of
+/// strings (char**) using metkit_marsrequest_params.
+/// I think we should metkit_paramiterator_t.
+struct metkit_paramiterator_t {


Lifetimes of returned values in this iterator differ from the metkit_requestiterator_t iterator. For itself the lifetime is ok (if documented) but a subtle difference in lifetime handling with both types imply symmetrical behavior is a nasty trap. metkit_requestiterator_t result lifetimes exceed the iterator due to the move out of the iterator, while the char* from this iterator dangle as soon as the iterator is freed.

src/metkit/api/metkit_c.cc

Ozaq · 2024-12-19T14:57:05Z

src/metkit/api/metkit_c.cc

+        return METKIT_ERROR_USER;
+    }
+    catch (const eckit::AssertionFailed& e) {
+        eckit::Log::error() << "Assertion Failed: " << e.what() << std::endl;


I think the c api wrapper should not log errors, this is imo a responsibility of metkit itself -OR- the calling code.

I'm inclined to agree

Ozaq · 2024-12-19T14:58:00Z

src/metkit/api/metkit_c.cc

+int metkit_version(const char** version) {
+    *version = metkit_version_str();
+    return METKIT_SUCCESS;
+}
+
+int metkit_vcs_version(const char** sha1) {
+    *sha1 = metkit_git_sha1();
+    return METKIT_SUCCESS;
+}


So since its always METKIT_SUCCESS this should just return the const char*

src/metkit/api/metkit_c.cc

ChrisspyB · 2024-12-20T16:20:55Z

You need an extra test where you use the API and compile the test with a C compiler

Maybe we should rewrite test_metkit_c.cc in C? For the time being I'll add a simple test that at least uses a C compiler

ChrisspyB · 2024-12-20T16:36:24Z

Re: error handling, if we're going to do similar across the stack, it might make sense to centralise this somewhere (eckit). All of the caught exceptions are in fact eckit::exceptions

jinmannwong and others added 6 commits January 24, 2024 13:20

Initial C API for Python interface

53d5324

Add comments

5bdb1e1

clang format

e7523ae

Review comments and expand API

9a2313d

Add merge to C API

8549509

Merge branch 'bitbucket_jenny/feature/metkit-python' into feature/c-api

d066c18

ChrisspyB added 5 commits December 11, 2024 15:33

Add some C-API test coverage, and add request_values

1b03851

Rename metkit_request to metkit_marsrequest

3907511

(c api) - expand test coverage, fix iterator skipping first element

e050cd5

Refactor private members and move semantics

fd1d8b1

Add header

bf9f488

ChrisspyB requested a review from tbkr December 16, 2024 09:43

ChrisspyB marked this pull request as ready for review December 16, 2024 09:43

ChrisspyB commented Dec 16, 2024

View reviewed changes

ChrisspyB requested review from simondsmart and MircoValentiniECMWF December 17, 2024 15:57

tbkr requested changes Dec 18, 2024

View reviewed changes

simondsmart requested changes Dec 18, 2024

View reviewed changes

Ozaq reviewed Dec 19, 2024

View reviewed changes

src/metkit/api/metkit_c.cc Outdated Show resolved Hide resolved

Respond to subset of review comments

1b324a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/c api #57

Feature/c api #57

ChrisspyB commented Dec 11, 2024

codecov-commenter commented Dec 11, 2024 •

edited

Loading

ChrisspyB Dec 16, 2024

tbkr Dec 18, 2024

tbkr left a comment

tbkr Dec 18, 2024

ChrisspyB Dec 20, 2024

tbkr Dec 18, 2024

tbkr Dec 18, 2024

tbkr Dec 18, 2024

tbkr Dec 18, 2024

simondsmart Dec 18, 2024

simondsmart Dec 18, 2024

simondsmart Dec 18, 2024

ChrisspyB Dec 20, 2024

simondsmart Dec 18, 2024

ChrisspyB Dec 18, 2024

Ozaq Dec 19, 2024

Ozaq left a comment

Ozaq Dec 19, 2024

Ozaq Dec 19, 2024

ChrisspyB Dec 20, 2024

Ozaq Dec 19, 2024

Ozaq Dec 19, 2024

Ozaq Dec 19, 2024

Ozaq Dec 19, 2024

Ozaq Dec 19, 2024

Ozaq Dec 19, 2024

ChrisspyB Dec 20, 2024

Ozaq Dec 19, 2024

ChrisspyB commented Dec 20, 2024 •

edited

Loading

ChrisspyB commented Dec 20, 2024


		} // extern "C"

		static thread_local std::string g_current_error_string;

Feature/c api #57

Are you sure you want to change the base?

Feature/c api #57

Conversation

ChrisspyB commented Dec 11, 2024

codecov-commenter commented Dec 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ozaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisspyB commented Dec 20, 2024 • edited Loading

ChrisspyB commented Dec 20, 2024

codecov-commenter commented Dec 11, 2024 •

edited

Loading

ChrisspyB commented Dec 20, 2024 •

edited

Loading