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

chore: http client healthcheck #3636

Merged
merged 8 commits into from
Mar 8, 2023
Merged

chore: http client healthcheck #3636

merged 8 commits into from
Mar 8, 2023

Conversation

denyszhak
Copy link
Contributor

What does this PR address?

Fixes #3315

Before submitting:

@denyszhak denyszhak requested a review from a team as a code owner March 4, 2023 16:04
@denyszhak denyszhak requested review from sauyon and removed request for a team March 4, 2023 16:04
Copy link
Contributor

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

LGTM. Would kindly request if you can write a quick test for this.

You can write the test under any of the test/e2e/bento_[grpc|http]_server.

@codecov
Copy link

codecov bot commented Mar 4, 2023

Codecov Report

Merging #3636 (04d81fa) into main (45cf38a) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3636      +/-   ##
==========================================
+ Coverage   31.86%   31.98%   +0.12%     
==========================================
  Files         146      146              
  Lines       12014    12039      +25     
  Branches     1977     1989      +12     
==========================================
+ Hits         3828     3851      +23     
+ Misses       7917     7915       -2     
- Partials      269      273       +4     
Impacted Files Coverage Δ
src/bentoml/_internal/client/http.py 39.60% <100.00%> (+4.49%) ⬆️
src/bentoml/_internal/configuration/v1/__init__.py 47.72% <0.00%> (-1.11%) ⬇️
src/bentoml/_internal/configuration/containers.py 47.57% <0.00%> (-0.86%) ⬇️
src/bentoml/_internal/io_descriptors/pandas.py 39.23% <0.00%> (-0.44%) ⬇️
src/bentoml/_internal/io_descriptors/numpy.py 42.59% <0.00%> (-0.40%) ⬇️
src/bentoml/bentos.py 36.58% <0.00%> (ø)
src/bentoml/models.py 77.04% <0.00%> (ø)
src/bentoml/exceptions.py 87.50% <0.00%> (ø)
src/bentoml/_internal/frameworks/keras.py 0.00% <0.00%> (ø)
src/bentoml/_internal/server/runner_app.py 0.00% <0.00%> (ø)
... and 7 more

@denyszhak
Copy link
Contributor Author

denyszhak commented Mar 7, 2023

LGTM. Would kindly request if you can write a quick test for this?

You can write the test under any of the test/e2e/bento_[grpc|http]_server.

WDYT if I move simple_service feature on the level above unit and e2e tests? or do you want me to create a separate Service instance for client test

@aarnphm
Copy link
Contributor

aarnphm commented Mar 7, 2023

Lets keep the simple_service fixture in unit, and you can write a simple client test under e2e instead.

No need to create a separate Service instance. The e2e service test will setup everything.

@denyszhak
Copy link
Contributor Author

Lets keep the simple_service fixture in unit, and you can write a simple client test under e2e instead.

No need to create a separate Service instance. The e2e service test will setup everything.

Thanks, that makes sense, didn't know the load() API, please check

Copy link
Contributor

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

Some finals comments. Otherwise LGTM

tests/e2e/bento_server_http/tests/test_client.py Outdated Show resolved Hide resolved
Copy link
Contributor

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

@aarnphm aarnphm merged commit 7d14975 into bentoml:main Mar 8, 2023
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.

feature: client.health
2 participants