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

Added test cases for SageMaker MME requirements #2200

Merged
merged 37 commits into from
Apr 4, 2023

Conversation

agunapal
Copy link
Collaborator

@agunapal agunapal commented Mar 24, 2023

Description

Added 2 test cases for SageMaker MME requirements

  • Model not loaded (404)
  • OOM Error (507

APIs defined here

Fixes #(issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Feature/Issue validation/testing

Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

  • PyTest
python -m pytest -v test_sm_mme_requirements.py 
============================================================================= test session starts =============================================================================
platform linux -- Python 3.8.16, pytest-7.2.2, pluggy-1.0.0 -- /home/ubuntu/anaconda3/envs/torchserve/bin/python
cachedir: .pytest_cache
rootdir: /home/ubuntu/serve
plugins: cov-4.0.0, mock-3.10.0
collected 3 items                                                                                                                                                             

test_sm_mme_requirements.py::test_no_model_loaded PASSED                 [ 33%]
test_sm_mme_requirements.py::test_oom_on_model_load PASSED               [ 66%]
test_sm_mme_requirements.py::test_oom_on_invoke PASSED                   [100%]

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?

@agunapal agunapal requested a review from lxning March 24, 2023 18:02
@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #2200 (3e86d7f) into master (98ca286) will decrease coverage by 0.16%.
The diff coverage is 0.00%.

❗ Current head 3e86d7f differs from pull request most recent head 967a5f7. Consider uploading reports for the commit 967a5f7 to get more accurate results

@@            Coverage Diff             @@
##           master    #2200      +/-   ##
==========================================
- Coverage   71.52%   71.37%   -0.16%     
==========================================
  Files          73       73              
  Lines        3329     3336       +7     
  Branches       57       57              
==========================================
  Hits         2381     2381              
- Misses        945      952       +7     
  Partials        3        3              
Impacted Files Coverage Δ
ts/model_service_worker.py 65.71% <0.00%> (-1.94%) ⬇️
ts/service.py 71.60% <0.00%> (-2.76%) ⬇️

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

ts/service.py Show resolved Hide resolved
@agunapal
Copy link
Collaborator Author

@lxning

Now on model loading, I am getting code 507,

but the message is as follows

{
  "code": 507,
  "type": "InternalServerException",
  "message": "Failed to start workers for model BERTSeqClassification version: 1.0"
}

This happens because here we are setting this. I don't know if we want to fix this in this PR.

@lxning
Copy link
Collaborator

lxning commented Mar 28, 2023

@agunapal reply to question:

let's add sth like this at https://github.com/pytorch/serve/blob/master/frontend/server/src/main/java/org/pytorch/serve/util/ApiUtils.java#L283:
if (v == 507) {
String msg = "outofmemory"
} else {
}

@agunapal agunapal changed the title (WIP) Added 2 test cases for SageMaker MME requirements (WIP) Added test cases for SageMaker MME requirements Mar 28, 2023
@agunapal agunapal requested a review from lxning March 28, 2023 20:30
@agunapal
Copy link
Collaborator Author

@lxning Looks like that wasn't the right place. v value was 413 there. We can perhaps address this in another PR in interest of time?

.github/workflows/regression_tests_cpu.yml Outdated Show resolved Hide resolved
ts/model_service_worker.py Outdated Show resolved Hide resolved
test/pytest/test_sm_mme_requirements.py Show resolved Hide resolved
test/pytest/test_sm_mme_requirements.py Show resolved Hide resolved
ts/model_service_worker.py Show resolved Hide resolved
@msaroufim msaroufim self-requested a review April 3, 2023 23:55
@agunapal agunapal changed the title (WIP) Added test cases for SageMaker MME requirements Added test cases for SageMaker MME requirements Apr 4, 2023
@agunapal agunapal merged commit a4a1f65 into master Apr 4, 2023
@agunapal agunapal deleted the feature/regression_test_sm_mme branch April 4, 2023 01:09
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.

None yet

4 participants