C API Support #1626

youwuyou · 2024-06-18T10:34:45Z

This PR adds the initial support of the C API implementation for interoperating with other programming languages, such as Fortran, Python, and Julia. The language linkage with key word extern "c" is used to enable the interfacing.

C API Design:

Conveys an uniform naming convention as specified here
Usage of macros and std::dynamic_pointer_cast to avoid code duplication

Changes:

Added header file include/ginkgo/c_api.h (define macros, declares function/structs) and source file core/c_api.cpp (concretize definitions of declared functions that internally calls ginkgo)
Necessary changes in CMake files cmake/install_helpers.cmake and core/CMakeLists.txt to include the C API

Example Usage:

Calling ginkgo from C through the C API can be seen here
The Julia package Ginkgo.jl calls ginkgo routines with ccall through the from Clang generated Julia side api.jl, which internally executes compiled dynamic libraries of ginkgo. It supports both locally compiled dynamic libraries or ones that are hosted under an artifact package ginkgo_jll.jl.

…stallation dir

…to pasc_update

Initial API support for iterative solvers with preconditioner, factorization and direct solvers

MarcelKoch

Thanks for the work, this looks already quite good! I left some remarks below.

The big points I want to bring up are:

executor function names. I'm not sure _cpu, _gpu parts are necessary for executor function names. From the rest of the name, it is already implied if it is for a cpu or gpu executor or not. Also this implies a type hierarchy level that is not really present in ginkgo at the moment.
naming consistency. Should the prefix be ginkgo or gko. I might tend to gko, since that matches our namespace.
error handling. A lot of Ginkgo functions can throw, and I'm unsure what will happen on the C side in that case. I guess the standard approach would be to catch the exception and return error codes instead.

MarcelKoch · 2024-06-21T09:33:25Z

include/ginkgo/c_api.h

+ * @param _name  Name of the datatype of the array
+ *
+ */
+#define DEFINE_ARRAY_OVERLOAD(_ctype, _cpptype, _name)                         \


I'm not sure the define macros should be placed in the header. They add information that is unnecessary for users, and which they should not rely on.

True, since they are only required in c_api.cpp, they could also be defined there.

In addition to putting them in c_api.cpp, I would also recommend prefixing all of them with GKO_.

MarcelKoch · 2024-06-21T09:34:28Z

include/ginkgo/c_api.h

+                                                                               \
+    size_t ginkgo_array_##_name##_get_size(gko_array_##_name array_st_ptr)     \
+    {                                                                          \
+        return (*array_st_ptr).arr.get_size();                                 \


Am I missing something or could the (*struct).member syntax be simplified:

Suggested change

return (*array_st_ptr).arr.get_size(); \

return array_st_ptr->arr.get_size(); \

MarcelKoch · 2024-06-21T09:41:38Z

include/ginkgo/c_api.h

+void ginkgo_linop_apply(gko_linop A_st_ptr, gko_linop b_st_ptr,
+                        gko_linop x_st_ptr);


how does this work with b and x as vectors? Is there a way to convert the ginkgo_matrix_dense_f32 to a gko_linop?

MarcelKoch · 2024-06-21T09:42:16Z

include/ginkgo/c_api.h

+ * parameter
+ *
+ */
+struct gko_deferred_factory_parameter_st;


Why does this use gko_ instead of ginkgo_? Same for the other instances below.

I was thinking to use gko_ for struct objects and ginkgo_ for actual methods, and thus I used gko_linop, gko_deferred_factory_parameter, gko_executor etc.

I think it's more common for c libraries to have a single prefix that they stick to.

Makes sense! Probably we should opt for using just gko_ then?

That is what I would prefer.

MarcelKoch · 2024-06-21T09:43:59Z

include/ginkgo/c_api.h

+                        gko_linop x_st_ptr);
+
+//-------------------- Iterative solvers -----------------------------
+gko_linop ginkgo_linop_cg_preconditioned_f64_create(


I think I would prefer

Suggested change

gko_linop ginkgo_linop_cg_preconditioned_f64_create(

gko_linop ginkgo_solver_cg_preconditioned_f64_create(

since that matches again the namespace.
Also, I would leave out preconditioned from the name, since there is no non-preconditioned version.

MarcelKoch · 2024-06-21T12:28:50Z

include/ginkgo/c_api.h

+/* ----------------------------------------------------------------------
+ * C memory management
+ * ---------------------------------------------------------------------- */
+void c_char_ptr_free(char* ptr);


what is this doing here? It doesn't seem to be related to ginkgo.

I was experimenting a bit around the I/O interfacing and tried to have dense matrices printed using _write_mtx (defined under macro).

If you need this as a public function, it must be prefixed (for functions, you seem to use ginkgo_).

include/ginkgo/c_api.h

MarcelKoch · 2024-06-24T07:20:43Z

core/c_api.cpp

+    if (auto omp_exec = std::dynamic_pointer_cast<gko::OmpExecutor>(
+            exec_st_ptr->shared_ptr)) {
+        return new gko_executor_st{
+            gko::CudaExecutor::create(device_id, exec_st_ptr->shared_ptr)};
+    } else if (auto reference_exec =
+                   std::dynamic_pointer_cast<gko::ReferenceExecutor>(
+                       exec_st_ptr->shared_ptr)) {
+        return new gko_executor_st{
+            gko::CudaExecutor::create(device_id, exec_st_ptr->shared_ptr)};


could be combined

Suggested change

if (auto omp_exec = std::dynamic_pointer_cast<gko::OmpExecutor>(

exec_st_ptr->shared_ptr)) {

return new gko_executor_st{

gko::CudaExecutor::create(device_id, exec_st_ptr->shared_ptr)};

} else if (auto reference_exec =

std::dynamic_pointer_cast<gko::ReferenceExecutor>(

exec_st_ptr->shared_ptr)) {

return new gko_executor_st{

gko::CudaExecutor::create(device_id, exec_st_ptr->shared_ptr)};

if ( std::dynamic_pointer_cast<gko::OmpExecutor>(

exec_st_ptr->shared_ptr) ||

std::dynamic_pointer_cast<gko::ReferenceExecutor>(

exec_st_ptr->shared_ptr)) {

return new gko_executor_st{

gko::CudaExecutor::create(device_id, exec_st_ptr->shared_ptr)};

same for hip and sycl

MarcelKoch · 2024-06-24T07:26:22Z

core/c_api.cpp

+    }
+}
+
+size_t ginkgo_executor_cuda_get_num_devices()


this should return an int instead of size_t, since that's what the ginkgo function returns.
Or is the size_t required from the julia side.
This also applies to many of the executor functions.

MarcelKoch · 2024-06-24T07:39:55Z

include/ginkgo/c_api.h

+/* ----------------------------------------------------------------------
+ * Library functions for creating arrays and array operations in GINKGO
+ * ---------------------------------------------------------------------- */
+DECLARE_ARRAY_OVERLOAD(int16_t, int16_t, i16)


I don't think the int16 overload is useful. It can't be used to create matrices, or basically anything relevant. Instead, a size_t might be more helpful, although this is also not necessary for the functions currently available in the C-API.

greole

Just some quick comments

core/c_api.cpp

include/ginkgo/c_api.h

greole · 2024-06-24T13:09:33Z

include/ginkgo/c_api.h

+#ifndef C_API_H
+#define C_API_H


Suggested change

#ifndef C_API_H

#define C_API_H

#ifndef GKO_C_API_H

#define GKO_C_API_H

Maybe better to prefix with GKO to avoid potential collisions.

greole · 2024-06-24T13:19:13Z

include/ginkgo/c_api.h

+ * @param _name  Name of the datatype of the array
+ *
+ */
+#define DEFINE_ARRAY_OVERLOAD(_ctype, _cpptype, _name)                         \


True, since they are only required in c_api.cpp, they could also be defined there.

greole · 2024-06-24T13:21:55Z

include/ginkgo/c_api.h

+ * its construction by removing the repetitive typing of array's name.
+ *
+ * @param _ctype_value  Type name of the element type in C
+ *


Suggested change

*

We rather keep all the @param together instead of separating them by an empty line.

Thank you! I will edit the ones that do not match the format

greole · 2024-06-24T13:23:55Z

include/ginkgo/c_api.h

+ * construction by removing the repetitive typing of array's name.
+ *
+ * @param _ctype  Type name of the element type in C
+ *


Suggested change

*

We rather keep the @param together instead of separating them by an empty line.

remove redundant author comments Co-authored-by: Gregor Olenik <gregor.olenik@web.de>

thoasm

Good work in general!
Most of my comments are about names and moving things to the CPP file.
Also, you seem to mix the prefix gko_ (mostly structs) and ginkgo_ (functions).
I don't think we need to make a distinction here,
I would recommend to use gko_ for everything (and GKO_ for macros), as you have already discussed with @MarcelKoch in the comments.

I need to read up a bit more on C APIs to give my final review, so consider this 1/2.

thoasm · 2024-07-02T15:46:36Z

core/c_api.cpp

+//
+// SPDX-License-Identifier: BSD-3-Clause
+
+#include <../include/ginkgo/c_api.h>


<> means it's in one of the header directories, so this should be:

Suggested change

#include <../include/ginkgo/c_api.h>

#include <ginkgo/c_api.h>

Alternatively, if you want to specify the relative path, use regular quotations:

#include "../include/ginkgo/c_api.h"

thoasm · 2024-08-22T12:46:53Z

include/ginkgo/c_api.h

+ * @param _name  Name of the datatype of the array
+ *
+ */
+#define DEFINE_ARRAY_OVERLOAD(_ctype, _cpptype, _name)                         \


In addition to putting them in c_api.cpp, I would also recommend prefixing all of them with GKO_.

thoasm · 2024-08-22T12:47:47Z

include/ginkgo/c_api.h

+ * @brief A build instruction for declaring gko::array<T> in the C API header
+ * file
+ */
+#define DECLARE_ARRAY_OVERLOAD(_ctype, _cpptype, _name)                       \


This macro must have a GKO_ prefix because it's a public-facing header.

thoasm · 2024-08-22T12:49:13Z

include/ginkgo/c_api.h

+ * @param _name  Name of the datatype of the dense matrix
+ *
+ */
+#define DEFINE_DENSE_OVERLOAD(_ctype, _cpptype, _name)                      \


Prefix with GKO_ and put it into the c_api.cpp file.

thoasm · 2024-08-23T00:07:54Z

include/ginkgo/c_api.h

+ * @brief A build instruction for declaring gko::matrix::Dense<T> in the C API
+ * header file
+ */
+#define DECLARE_DENSE_OVERLOAD(_ctype, _cpptype, _name)                      \


GKO_ Prefix

thoasm · 2024-08-23T00:08:38Z

include/ginkgo/c_api.h

+ * @param _name_dense Name of the datatype of the dense matrix for apply
+ * function
+ */
+#define DEFINE_CSR_OVERLOAD(_ctype_value, _ctype_index, _cpptype_value,        \


GKO_ and move it to c_api.cpp

thoasm · 2024-08-23T00:09:37Z

include/ginkgo/c_api.h

+ * @brief A build instruction for declaring gko::matrix::Csr<Tv,Ti> in the C API
+ * header file
+ */
+#define DECLARE_CSR_OVERLOAD(_ctype_value, _ctype_index, _cpptype_value,       \


GKO_ prefix

thoasm · 2024-08-23T00:10:43Z

include/ginkgo/c_api.h

+/* ----------------------------------------------------------------------
+ * C memory management
+ * ---------------------------------------------------------------------- */
+void c_char_ptr_free(char* ptr);


If you need this as a public function, it must be prefixed (for functions, you seem to use ginkgo_).

thoasm · 2024-08-23T09:21:45Z

I also have a general question: Should we add some form of CI to check if the API is still working?

+ From #1626

greole · 2024-11-06T14:08:29Z

include/ginkgo/c_api.h

I guess I would prefer this file to be in include/ginkgo/extension folder.

youwuyou and others added 23 commits November 4, 2023 00:32

added c api, work on progress.

c62d1ea

Merge branch 'ginkgo-project:develop' into develop

d3678fe

changed the c api header name to make c_api.hpp be included in the in…

9b6b383

…stallation dir

changed entry of the suffix to be detected instead for the c api

99052c3

propagate changes to c api

c4128a2

Merge branch 'ginkgo-project:develop' into develop

c8773d4

added support for more eltypes of csr matrix

b23d0bc

removed redundant semicolons

b8e1e82

include the header for c integer types

d9da248

Merge branch 'ginkgo-project:develop' into develop

5b450a0

minor changes

37d66b4

Merge branch 'develop' of https://github.com/ginkgo-project/ginkgo in…

6111a83

…to pasc_update

Merge pull request #2 from youwuyou/pasc_update

b68b24f

Initial API support for iterative solvers with preconditioner, factorization and direct solvers

added sketch of direct & preconditioned iterative solvers

20a1068

used dynamic cast to go down of inheritance

d992285

use dynamic cast in cuda/hip getters from executors

03d72ab

added more gpu getters

b260d76

refined doxygen and gpu getter routines

ef134ca

Merge branch 'pasc_update' of github.com:youwuyou/ginkgo into develop

a4ff795

Merge branch 'ginkgo-project:develop' into develop

9f52779

Merge branch 'develop' of github.com:youwuyou/ginkgo into develop

2c595d7

remove unnecessary comments

fcdec96

revert new line change to cmakelist

9f1434f

youwuyou self-assigned this Jun 18, 2024

youwuyou requested a review from a team June 18, 2024 10:49

MarcelKoch requested changes Jun 24, 2024

View reviewed changes

greole reviewed Jun 24, 2024

View reviewed changes

youwuyou and others added 2 commits June 24, 2024 16:15

Update include/ginkgo/c_api.h

dcd0f9b

remove redundant author comments Co-authored-by: Gregor Olenik <gregor.olenik@web.de>

Update core/c_api.cpp

dcf6ae9

remove redundant author comments Co-authored-by: Gregor Olenik <gregor.olenik@web.de>

thoasm reviewed Aug 23, 2024

View reviewed changes

MarcelKoch added this to the Ginkgo 1.9.0 milestone Aug 26, 2024

pratikvn added a commit that referenced this pull request Nov 5, 2024

Copy @youwuyou's C API code

64ca8d4

+ From #1626

MarcelKoch modified the milestones: Ginkgo 1.9.0, Ginkgo 1.10.0 Nov 6, 2024

greole reviewed Nov 6, 2024

View reviewed changes

include/ginkgo/c_api.h

Copy link

Collaborator

greole Nov 6, 2024 •

edited

Loading

Choose a reason for hiding this comment

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

I guess I would prefer this file to be in include/ginkgo/extension folder.

	return (*array_st_ptr).arr.get_size(); \
	return array_st_ptr->arr.get_size(); \

		void ginkgo_linop_apply(gko_linop A_st_ptr, gko_linop b_st_ptr,
		gko_linop x_st_ptr);

	gko_linop ginkgo_linop_cg_preconditioned_f64_create(
	gko_linop ginkgo_solver_cg_preconditioned_f64_create(

	#include <../include/ginkgo/c_api.h>
	#include <ginkgo/c_api.h>

C API Support #1626

Are you sure you want to change the base?

C API Support #1626

Conversation

youwuyou commented Jun 18, 2024 • edited Loading

MarcelKoch 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

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

greole 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

thoasm 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

thoasm commented Aug 23, 2024

greole Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

youwuyou commented Jun 18, 2024 •

edited

Loading

greole Nov 6, 2024 •

edited

Loading