Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement openAI endpoint invoker for nuget #15797
Implement openAI endpoint invoker for nuget #15797
Changes from 7 commits
a1feb54
dfc5461
8aaac2f
6cdae88
5c7c67c
842f7fd
1fec44a
a6f18e3
9c6bb2e
7e14fd6
c0e72cc
e558c32
6b88835
dc7ae1f
94279c1
2973579
952eb52
ae7d534
7e51a9c
fdad7c7
c013693
e0dcb22
866db7e
d099bd6
17ae944
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even through curl_global_init is thread-safe, we still should not call it from any global variable's constructor since at that time the static variables in libcurl itself might have not been initialized. So I think the best way to ensure this is to put this call in OrtEnv's constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't follow, how come curl statics are not initialized on OpenAIInvoker::OpenAIInvoker(...), while OrtEnv constructor could avoid that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked with Randy offline. The most difficult part is: this file also uses TritonClient lib which also has similar code that initialize and deinit curl. I don't see an easy way to coordinate them together. Will leave the discussion later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you need a std::string, instead of stringstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stringstream buffers extra space from coming contents, which is more suitable for the scenario here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't std::string do the same thing? Every string has a
size
andcapacity
. Capacity means how much memory has been allocated for this string. Size means how much memory that has been used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stringstream is designd for buffering content grows dynamically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the C style callbacks should not throw exceptions, since C could not handle C++ exceptions. So all such callbacks need be marked as nothrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if c++ exceptions throw from the callback, wouldn't ort be the catcher?
For sure it's none of curl's business.
Another thing, suppose exception for some reason indeed occurred in write-callback, wouldn't it be mostly appropriate to throw it to the upper logic since the response is totally busted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sdnow returns CURLE_WRITE_ERROR on exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may change it to a std::unique_ptr. See https://stackoverflow.com/questions/27440953/stdunique-ptr-for-c-functions-that-need-free for an example. Instead of using std::free to free the memory, in your case you need to use
curl_easy_cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine but we got to call curl_easy_cleanup whatsoever in destructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We see std::unique_ptr is safer than raw pointers, which means your current code is not wrong, but using std::unique_ptr could make the code easier to read and verify. Just a suggestion. You don't have to take it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just emplace_back
output_tensor
to the vector?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.