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

Add higher level tritonserver API #1361

Merged
merged 57 commits into from
Jun 27, 2023

Conversation

jbkyang-nvi
Copy link
Contributor

With https://github.com/triton-inference-server/developer_tools/tree/main/server,
Triton added a C++ API that is higher level API for the original API.
This PR adds tritonserverwrapper project that makes use of that API. It is higher than the
Javacpp/tritonserverwrapper project

@saudet
Copy link
Member

saudet commented May 25, 2023

Thanks! Instead of adding a new module though, I think we could simply update the original tritonserver one with the new "tritonserverwrapper"? BTW, we typically name these presets after the library names, which is in this case "tritondevelopertoolsserver"...

@jbkyang-nvi
Copy link
Contributor Author

Thanks! Instead of adding a new presets though, I think we could simply update the original tritonserver one with the new "tritonserverwrapper"? BTW, we typically these modules after the library names, which is in this case "tritondevelopertoolsserver"...

tritonserver wraps around the CAPI and the new tritondevelopertoolsserver API wraps around a higher level version of that. I think both work a little differently and it would be nice to have them as different projects, if that makes sense.

@saudet
Copy link
Member

saudet commented May 25, 2023 via email

@jbkyang-nvi
Copy link
Contributor Author

Updated the PR. I noticed there's some issues caused by updating the tritonserver.h from 23.04 to 23.05 though comparison link. Through this PR: triton-inference-server/core#192 we made tritonserver.h conform to proper C standard (vs C++ standard).

tritonserver.java created seemed to be confused and ommited a few functions that used defined variables.: 63431c6#diff-0b23a69d7a0bd76ec9fff99224228855a84574ccaf9185df0c5d774ba00dc933

I tried to use cpptext and it also did not solve the problem locally. Is there anything else I should be doing? I checked, and 23.04 seems to be compiling fine.
e90149b

@saudet
Copy link
Member

saudet commented Jun 10, 2023

Right now it's not failing because of that, but because of this:

2023-06-10T02:51:14.7189856Z ../cppbuild.sh: line 10: INCLUDE_DEVELOPER_TOOLS_SERVER: unbound variable

Let's start by fixing this

saudet added a commit to bytedeco/javacpp that referenced this pull request Jun 10, 2023
@saudet
Copy link
Member

saudet commented Jun 10, 2023

I've fixed the parser issue with function pointers in commit bytedeco/javacpp@b9f91d7 so it should work fine now.

@jbkyang-nvi
Copy link
Contributor Author

@saudet Thanks for your help! It looks like samples/Simple.java is running fine but the example for samples/SimpleCPP.java does not work. Is this something with how platform/pom.xml is written?

@saudet
Copy link
Member

saudet commented Jun 15, 2023

What do you mean by does not work? What's the error message that you get?

@jbkyang-nvi
Copy link
Contributor Author

What do you mean by does not work? What's the error message that you get?

It simply does not find the C++ binding files. I checked in the compiled jar and it seems like all the generated java files are in there

@saudet
Copy link
Member

saudet commented Jun 21, 2023

Could you run the sample with mvn clean compile exec:java -Dorg.bytedeco.java.logger.debug and copy/paste here what that outputs?

@jbkyang-nvi
Copy link
Contributor Author

jbkyang-nvi commented Jun 23, 2023

Could you run the sample with mvn clean compile exec:java -Dorg.bytedeco.java.logger.debug and copy/paste here what that outputs?

I think I found the reason of the failure. Thanks. Let me know if there's anything else that needs to be changed

@jbkyang-nvi
Copy link
Contributor Author

@saudet I've addressed the comments. Anything else to do before merging to master?

@saudet
Copy link
Member

saudet commented Jun 27, 2023

Looks good! Thanks

@saudet saudet merged commit cb80fd6 into bytedeco:master Jun 27, 2023
@jbkyang-nvi jbkyang-nvi deleted the kyang-add-wrapper-to-build branch June 27, 2023 21:39
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