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

[C API] Initial interface proposal #396

Merged
merged 1 commit into from
Jul 17, 2020
Merged

[C API] Initial interface proposal #396

merged 1 commit into from
Jul 17, 2020

Conversation

VivekPanyam
Copy link
Collaborator

@VivekPanyam VivekPanyam commented Jul 9, 2020

Summary:

An initial interface for a Neuropod C API.

@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #396 into master will increase coverage by 0.61%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #396      +/-   ##
==========================================
+ Coverage   89.47%   90.09%   +0.61%     
==========================================
  Files          57       77      +20     
  Lines        3924     4733     +809     
==========================================
+ Hits         3511     4264     +753     
- Misses        413      469      +56     
Impacted Files Coverage Δ
source/python/neuropod/utils/eval_utils.py 81.39% <0.00%> (ø)
source/python/neuropod/packagers.py 100.00% <0.00%> (ø)
source/python/neuropod/backends/keras/packager.py 84.21% <0.00%> (ø)
...urce/python/neuropod/backends/neuropod_executor.py 93.05% <0.00%> (ø)
source/python/neuropod/utils/env_utils.py 100.00% <0.00%> (ø)
source/python/neuropod/backends/python/packager.py 91.30% <0.00%> (ø)
...e/python/neuropod/backends/torchscript/packager.py 85.71% <0.00%> (ø)
source/python/neuropod/utils/hash_utils.py 100.00% <0.00%> (ø)
...ource/python/neuropod/backends/pytorch/packager.py 100.00% <0.00%> (ø)
source/python/neuropod/backends/config_utils.py 94.87% <0.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef1c63f...61fe123. Read the comment docs.

@VivekPanyam VivekPanyam force-pushed the c_api branch 2 times, most recently from 24d25d3 to 0a1ce01 Compare July 10, 2020 00:29
@VivekPanyam
Copy link
Collaborator Author

Updated based on comments from @selitvin. Any thoughts @vkuzmin-uber?

extern "C" {
#endif

typedef struct NP_TensorAllocator NP_TensorAllocator;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow this and other declarations above for NP_Status, NP_Neuropod, NP_NeuropodTensor- is it just a stub and actual declarations will be added later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are the public facing header files (so these structs are opaque to users).

The definitions will be in _internal.h files which will be added in another PR

See the TensorFlow C API for an example of this pattern

const NP_NeuropodValueMap *inputs,
const char ** requested_outputs,
NP_NeuropodValueMap ** outputs,
NP_Status * status);
Copy link
Contributor

Choose a reason for hiding this comment

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

who is responsible for allocating and "free" "NP_Status * status" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The caller is responsible for doing so (using NP_NewStatus and NP_DeleteStatus)

@VivekPanyam VivekPanyam marked this pull request as ready for review July 15, 2020 19:43
@VivekPanyam VivekPanyam merged commit d7bf8b0 into master Jul 17, 2020
@VivekPanyam VivekPanyam deleted the c_api branch July 17, 2020 07:25
@VivekPanyam VivekPanyam linked an issue Aug 6, 2020 that may be closed by this pull request
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.

Add a C API to Neuropod
2 participants