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
Change httpclient to async #1958
Change httpclient to async #1958
Changes from 54 commits
bccfd16
425a450
0600d63
864e4bb
2fe466b
fa8d3ea
b761a5d
02f9602
3ee19ce
136830c
2941daf
6b54018
0bd49aa
5ecf3f2
a29b851
6ff57d9
f0ef57e
caf05a0
cadb989
d8a1281
c7e0588
e59e68c
42d08f8
f100187
e221284
3f0a252
802c537
3c76c22
7df33cd
ffd8565
3199a28
3eb3b7f
80f4c1f
7de1ab4
0ee363a
05a3111
b54c6b1
6a6287e
e3e98a1
8ffe6d7
bebb913
25d6845
031ab19
421b00b
71f3942
20a6172
80b91ed
e446f49
e138d83
f0f3a5b
b3aed63
d12fb86
5e6a7f6
a7a980c
ea2d860
ec8404a
13dc670
05d14bc
005fcd6
0ff85a8
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.
Codecov shows this default exception is not covered. Also what is the necessity here to default an exception? When the method is not implemented, the compile would fail first?
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.
Added tests for this. No, this is a runtime error, compilation won't be impacted by this. Below are reasons:
First of all, this is an interface method but not all subcalsses need to implement this method, e.g. RemoteModel. So we need to make it a default method, and this method can be accessed in all its implemented classes. Also this method needs return value, there are three options:
In fact, there's another option which is keep this method abstract and all subclasses needs to implement this and based on their case, they can throw UnsupportedOperationException which can also avoid accidentally abuse of the method, but this option we need to change quite a lot of files, and since this also works I choose this simpler option.
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.
This assumes all invoke remote model is a "Post" request by hardcoding. Very soon we will add more action types with different methods. So do you want to extend this to support more HTTP Method through different action types?
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.
This change is based on the existing code: https://github.com/opensearch-project/ml-commons/blob/main/ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/AwsConnectorExecutor.java#L116, I think when we needs to support more different methods, we can change this at that time, current implementation can adapt to that change easily.
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.
Is there any chance of
optionalContentLength
to be null? if yes then it might skip this checking?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.
No, even when payload is null, we create requestBody with
RequestBody.empty()
which is not null, and I use this method on purpose to make sure the requestBody never be null.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.
Is this mandatory ? We don't have this part before
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.
Currently no LLM accepts empty request body, adding this is an enhancement to our code which means we should have this in the first place.
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.
Not quite understand, if need to check request body, you can check body length directly, why have to add a header ?
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.
Aasync httpclient doesn't set the
Content-Length
automatically, we need to do this manually, otherwise we're not able to get correct response from remote model side.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.
Let's add some comment about what is this class for?
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.
sure