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 custom model chunk upload #1

Open
wants to merge 19 commits into
base: custom_model_poc
Choose a base branch
from

Conversation

LEFTA98
Copy link

@LEFTA98 LEFTA98 commented Aug 24, 2022

Description

This pull request adds a new API call for uploading a chunk of a custom model to OpenSearch.

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.

@ylwu-amzn
Copy link
Owner

Add some test and make sure ./gradlew build can pass ?

Objects.requireNonNull(name);
Objects.requireNonNull(version);
Objects.requireNonNull(url);
Objects.requireNonNull(chunkNumber);
Copy link
Owner

Choose a reason for hiding this comment

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

Check if totalChunks is null or not?


private String name;
private Integer version;
private byte[] url;
Copy link
Owner

Choose a reason for hiding this comment

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

Seems url is not a good name, rename to content ?

MLUploadChunkInput mlUploadChunkInput = uploadModelRequest.getMlUploadInput();

try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {
MLTask mlTask = MLTask.builder()
Copy link
Owner

Choose a reason for hiding this comment

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

Seems mlTask not being used here. Remove it?

@VisibleForTesting
MLUploadModelChunkRequest getRequest(RestRequest request) throws IOException {
String name = request.param("name");
// System.out.println(name);
Copy link
Owner

Choose a reason for hiding this comment

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

Remove these debug code

String chunk_number = request.param("chunk_number");
// System.out.println(chunk_number);
String total_chunks = request.param("total_chunks");
byte[] content = request.content().streamInput().readAllBytes();
Copy link
Owner

Choose a reason for hiding this comment

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

This is risky if user upload a big chunk. Suggest checking the chunk size first.

@LEFTA98 LEFTA98 marked this pull request as draft September 2, 2022 17:29
@LEFTA98 LEFTA98 marked this pull request as ready for review September 2, 2022 17:30
@ylwu-amzn
Copy link
Owner

ylwu-amzn commented Sep 6, 2022

I see there are 17 commits in total now and there are 9 more commits after first round of review. That's not easy for code reviewer to review the new updates. Suggest keep the commit history clean, at least squash the small commits into one, or squash the latest 9 commits into one, so reviewer can only check the updates of one commit quickly.

This commit adds the following:
Unit and integration tests for ML custom model chunk upload
Reformatted some code for better readability

Signed-off-by: Thomas Ma [email protected]
…pensearch-2.2

merging to squish last 9 commits
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.

2 participants