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] Continue implementation #409

Merged
merged 4 commits into from
Aug 11, 2020
Merged

Conversation

vkuzmin-uber
Copy link
Contributor

Summary:

Continue with basic implementation of a C API for Neuropod. Issue #407

Test Plan:

Added a test that loads a model and fails, runs inference successfully and fails, and verifies the name and platform.

@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #409 into master will increase coverage by 0.31%.
The diff coverage is 96.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #409      +/-   ##
==========================================
+ Coverage   89.55%   89.86%   +0.31%     
==========================================
  Files          87       87              
  Lines        5024     5080      +56     
==========================================
+ Hits         4499     4565      +66     
+ Misses        525      515      -10     
Impacted Files Coverage Δ
source/neuropod/bindings/c/np_tensor.cc 75.00% <50.00%> (-25.00%) ⬇️
source/neuropod/bindings/c/c_api.cc 100.00% <100.00%> (ø)
source/neuropod/backends/tensorflow/tf_backend.cc 90.94% <0.00%> (+2.43%) ⬆️
source/neuropod/internal/neuropod_loader.cc 92.00% <0.00%> (+2.66%) ⬆️
source/neuropod/bindings/c/np_status.cc 81.81% <0.00%> (+27.27%) ⬆️

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 20b6e1d...5fdc025. Read the comment docs.

Copy link
Collaborator

@VivekPanyam VivekPanyam left a comment

Choose a reason for hiding this comment

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

Looks good overall!

@@ -29,19 +29,44 @@ extern "C" {

typedef struct NP_Neuropod NP_Neuropod;

// Load a model given a path
// Load a model given a path.
// status is set if user provided a valid pointer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure it's a good idea for status to be optional here (and in the other methods). Doing so might make this interface somewhat error prone.

It's obviously easier to pass in nullptr than to use and check am NP_Status, but I think we should force usage.

When I was working with the TF C API, it was somewhat inconvenient to have to deal with statuses everywhere, but it forced me to do error checking and I think that led to more robust user code (and a better debugging experience).

Any thoughts @selitvin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought abut some extra efficiency for Infer. "error handler" instead of status may be more efficient (it would never call it on success), but let's make it required for now.

Copy link
Collaborator

@VivekPanyam VivekPanyam left a comment

Choose a reason for hiding this comment

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

One more thing :)

@thuningxu
Copy link

Let's make sure we test with a C compiler.

@vkuzmin-uber
Copy link
Contributor Author

Let's make sure we test with a C compiler.

This is a very good point. Tensorflow also uses C11 to implement smoke test for its C API. Just published update for PR, see test_c_api.c.

Copy link
Collaborator

@VivekPanyam VivekPanyam left a comment

Choose a reason for hiding this comment

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

Looks good! Two minor comments and we should be good to go :)

Continue with basic implementation of a C API for Neuropod. Issue uber#407
Added a test that loads a model and fails, runs inference successfully and fails, and verifies the name and platform.
Copy link
Collaborator

@VivekPanyam VivekPanyam left a comment

Choose a reason for hiding this comment

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

LGTM!

@VivekPanyam VivekPanyam merged commit c16ad53 into uber:master Aug 11, 2020
vkuzmin-uber added a commit to vkuzmin-uber/neuropod that referenced this pull request Aug 17, 2020
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.

3 participants