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

Implement Test for ModelRequestEncoder #2580

Merged
merged 4 commits into from
Oct 5, 2023
Merged

Implement Test for ModelRequestEncoder #2580

merged 4 commits into from
Oct 5, 2023

Conversation

abergmeier
Copy link
Contributor

@abergmeier abergmeier commented Sep 10, 2023

Description

Implemented ModelRequestEncoderTest. Currently tests Inference protocol only.

Type of change

Increased Unittest coverage for inference.

Feature/Issue validation/testing

  • org.pytorch.serve.util.codec.ModelRequestEncoderTest.testEmpty
  • org.pytorch.serve.util.codec.ModelRequestEncoderTest.testSimple

Checklist:

  • Did you have fun?
  • Have you added tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

@abergmeier abergmeier marked this pull request as ready for review September 10, 2023 20:47
@mreso
Copy link
Collaborator

mreso commented Sep 14, 2023

Hi @abergmeier thanks for contributing this test! Could you please address the CI failure?

@abergmeier
Copy link
Contributor Author

Hi @abergmeier thanks for contributing this test! Could you please address the CI failure?

Well the CI seems to have quite a bit of noise to signal ratio.
From what I could make out it seems like there is some formatting problem.
The CI and docs seem to be not very specific what formatting is expected/how that formatting can be reached.
Also, other CI failures seem to stem from flakiness.

So I am not really confident, that I will be able to solve CI failures without some guidance.

@agunapal
Copy link
Collaborator

@abergmeier
Copy link
Contributor Author

@abergmeier Did you try building frontend https://github.com/pytorch/serve/tree/master/frontend

frontend on master does not even build/test successful on my Ubuntu 22.04. I assume there is implicit configuration/dependencies which are not present on my machine.

There is a formatter here https://github.com/pytorch/serve/blob/master/frontend/build.gradle#L45

Mabye I was not clear enough. I am no Java developer.
So for me to be able to produce a patch that is formatted in a way that you accept I assume I need a way of running a formatter on these files. I have no idea how I run the appropriate formatter.

@lxning
Copy link
Collaborator

lxning commented Sep 21, 2023

@abergmeier you can format java code via command:

cd frontend
./gradlew format

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Merging #2580 (cdbf9e3) into master (f10a071) will not change coverage.
The diff coverage is n/a.

❗ Current head cdbf9e3 differs from pull request most recent head 06aab73. Consider uploading reports for the commit 06aab73 to get more accurate results

@@           Coverage Diff           @@
##           master    #2580   +/-   ##
=======================================
  Coverage   72.39%   72.39%           
=======================================
  Files          85       85           
  Lines        3956     3956           
  Branches       58       58           
=======================================
  Hits         2864     2864           
  Misses       1088     1088           
  Partials        4        4           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@abergmeier
Copy link
Contributor Author

Can this PR land?

Copy link
Collaborator

@mreso mreso left a comment

Choose a reason for hiding this comment

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

LGTM

@mreso mreso added this pull request to the merge queue Oct 5, 2023
@mreso
Copy link
Collaborator

mreso commented Oct 5, 2023

Thanks @abergmeier a lot for contributing this test

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 5, 2023
@mreso mreso added this pull request to the merge queue Oct 5, 2023
Merged via the queue into pytorch:master with commit cf19d7c Oct 5, 2023
12 checks passed
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.

4 participants