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

[Feature] Split remote inference text list if its number exceeds user configured limitation #2455

Closed
wants to merge 3 commits into from

Conversation

chishui
Copy link
Contributor

@chishui chishui commented May 17, 2024

Description

We'd like to have a solution to cut texts into small batches if total number of them in a batch request exceeds the limitation

Issues Resolved

#2428

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Liyun Xiu <[email protected]>
@chishui chishui temporarily deployed to ml-commons-cicd-env May 20, 2024 02:49 — with GitHub Actions Inactive
@chishui chishui temporarily deployed to ml-commons-cicd-env May 20, 2024 02:49 — with GitHub Actions Inactive
@chishui chishui temporarily deployed to ml-commons-cicd-env May 20, 2024 02:49 — with GitHub Actions Inactive
@chishui chishui had a problem deploying to ml-commons-cicd-env May 20, 2024 02:49 — with GitHub Actions Error
@chishui chishui had a problem deploying to ml-commons-cicd-env May 20, 2024 02:49 — with GitHub Actions Failure
@chishui chishui had a problem deploying to ml-commons-cicd-env May 20, 2024 02:49 — with GitHub Actions Error
} else {
// if the originalOrder is not empty, reorder based on it
List<ModelTensors> sortedModelTensors = Arrays.asList(new ModelTensors[modelTensorsList.size()]);
assert (originalOrderIndexes.size() == modelTensors.length);
Copy link
Collaborator

@xinyual xinyual May 20, 2024

Choose a reason for hiding this comment

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

I think the originalOrderIndexes size is the doc number, while modelTensors size is request size. So it would cause error. A simple way to reproduce is to try predict a four size input and set connector max batch size to 2. We may need to fetch all tensor and re-create ModelTensors Again.

One more small detail here: What is the list of modelTensors size here we want to keep? If we keep the size equal to request number, it may confuse user because actually sort and modelTensors is not the actual remote response (we re-sort it). So maybe keep all tensor into one modelTensors can be a choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this, it is a bug, will fix in next revision

Signed-off-by: Liyun Xiu <[email protected]>
@chishui chishui had a problem deploying to ml-commons-cicd-env May 20, 2024 12:05 — with GitHub Actions Failure
@chishui chishui had a problem deploying to ml-commons-cicd-env May 20, 2024 12:05 — with GitHub Actions Error
@chishui chishui had a problem deploying to ml-commons-cicd-env May 20, 2024 12:05 — with GitHub Actions Error
@chishui chishui had a problem deploying to ml-commons-cicd-env May 20, 2024 12:06 — with GitHub Actions Error
@chishui chishui had a problem deploying to ml-commons-cicd-env May 20, 2024 12:06 — with GitHub Actions Failure
@chishui chishui had a problem deploying to ml-commons-cicd-env May 20, 2024 12:06 — with GitHub Actions Error
@@ -41,6 +45,9 @@
import org.opensearch.script.ScriptService;

public interface RemoteConnectorExecutor {
int DEFAULT_BATCH_SIZE = -1;
String MAX_BATCH_SIZE_KEY = "max_batch_size";
String STEP_SIZE_KEY = "input_docs_processed_step_size";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is to control the batch size , why need to add another variable max_batch_size ?

@chishui
Copy link
Contributor Author

chishui commented May 27, 2024

Will implement the logic from neural search side. Please refer to #2428 (comment)

Closing now

@chishui chishui closed this May 27, 2024
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