-
Notifications
You must be signed in to change notification settings - Fork 871
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
[WIP] Batch inferencing tests #1249
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Channel channel = TestUtils.connect(ConnectorType.MANAGEMENT_CONNECTOR, configManager); | ||
Assert.assertNotNull(channel); |
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 can be replaced by Channel channel = TestUtils.getManagementChannel(configManager);
@@ -157,13 +157,19 @@ public static void registerModel( | |||
String url, | |||
String modelName, | |||
boolean withInitialWorkers, | |||
boolean syncChannel) | |||
boolean syncChannel, | |||
int batchSize) | |||
throws InterruptedException { |
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 fixing will break the old test cases.
Here, function registerModel needs be overloaded. In other words, the original function registerModel should be kept; the new function registerModel call the old one.
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.
Ok got it
TestUtils.setResult(null); | ||
TestUtils.setLatch(new CountDownLatch(1)); | ||
|
||
TestUtils.registerModel(channel, "noop.mar", "err_success", true, false, batch_size=batch_size); |
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.
replace it with TestUtils.registerModel(channel, "noop.mar", "err_success", true, false, batch_size)
Assert.assertEquals( | ||
status.getStatus(), | ||
"Model \"success_batch\" Version: 1.0 registered with 1 initial workers"); | ||
|
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.
It seems that this assertion misses modelName. And also where is the definition for the message "success_batch"?
for(int i = 0; i < batch_size ; i ++) { | ||
DefaultFullHttpRequest req = | ||
new DefaultFullHttpRequest( | ||
HttpVersion.HTTP_1_1, HttpMethod.POST, "/predictions/err_success"); | ||
// req.content().writeCharSequence("data=invalid_output", CharsetUtil.UTF_8); | ||
HttpUtil.setContentLength(req, req.content().readableBytes()); | ||
req.headers() | ||
.set( | ||
HttpHeaderNames.CONTENT_TYPE, | ||
HttpHeaderValues.APPLICATION_X_WWW_FORM_URLENCODED); | ||
channel.writeAndFlush(req); | ||
|
||
TestUtils.getLatch().await(); | ||
Assert.assertEquals(TestUtils.getHttpStatus(), HttpResponseStatus.ACCEPTED); | ||
} |
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.
Why these 4 inference requests can confirm batching is working?
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.
Yeah thinking about this some more it doesn't confirm anything really. In the case of the Python backend I can create a a batch size counter in the preprocess handler and pass it all the way to the post process and then validate there that my batch size was correct. Not sure how to do the same for frontend yet
@@ -92,7 +92,7 @@ def run_inference_using_url_with_data(purl=None, pfiles=None, ptimeout=120): | |||
else: | |||
return response | |||
|
|||
def run_inference_using_url_with_data_json(purl=None, json_input=None, ptimeout=120): | |||
def run_inference_using_url_with_data_json(purl=None, pfiles=None, json_input=None, ptimeout=120): |
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.
Why "pfiles" is needed? Will this change break existing tests?
@Test( | ||
alwaysRun = true, | ||
dependsOnMethods = {"testSuccessBatch"}) | ||
public void testErrorBatch() throws InterruptedException { |
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.
Why "testPredictionMemoryError" is replaced with "testSuccessBatch"?
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.
So the dependencies here just determine the order in which tests are ran, it's not a real dependency. All tests in this file are dependent on each other without needing it. We could perhaps have a a separate PR where we remove unnecessary dependencies but I believe this may be a known limitation because can't run more than one torchserve instance at a time so can't run more than one torchserve test at a time
@Test( | ||
alwaysRun = true, | ||
dependsOnMethods = {"testPredictionMemoryError"}) |
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.
Why this test case depends on "testPredictionMemoryError"?
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.
They're not dependent, I believe dependsOnMethods
is used as a way to set the order in which tests are called
@@ -224,6 +224,27 @@ def test_kfserving_mnist_model_register_and_inference_on_valid_model_explain(): | |||
|
|||
assert np.array(json.loads(response.content)['explanations']).shape == (1, 1, 28, 28) | |||
test_utils.unregister_model("mnist") | |||
|
|||
def test_mnist_batch_inference(): |
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.
where is this test case called? And also what's the difference b/s test_mnist_batch_inference vs testSuccessBatch?
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.
Purely a frontend vs backend test - test is called if you run pytest here
('batch_size', str(batch_size)), | ||
('batch_delay', '10000') | ||
|
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 part should not be changed if the old registerModel (see following) is kept.
public static void registerModel(
Channel channel,
String url,
String modelName,
boolean withInitialWorkers,
boolean syncChannel)
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.
my bad - was under the impression that you can add arguments to the end of a function with optional values to make sure no backwards compatbility is broken
Closing for now |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Before this PR can be merged - we need to make sure that batch inferencing works fine without any test breaks #1244
As of now this PR has a test for
Next step is a
Open question - how to error check responses