-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Skip JS Tune integration test #4548
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
Conversation
Whats our path to addressing this gap in testing? |
Notebooks testing - Of course, need team input on this. |
This issue is arising due to instance type with GPU enabled not being picked? Can we use this : https://github.com/aws/sagemaker-python-sdk/blob/master/tests/integ/sagemaker/serve/test_serve_model_builder_gpu.py#L91 |
This reverts commit 627f394.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4548 +/- ##
==========================================
- Coverage 87.44% 87.43% -0.01%
==========================================
Files 389 389
Lines 36885 36885
==========================================
- Hits 32253 32252 -1
- Misses 4632 4633 +1 ☔ View full report in Codecov by Sentry. |
Yeah, this could work if we pass instance type in Passing instance type during model builder init doesn't help either
Note: As this test case was recently added. I'll completely remove it. |
Thank you so much! |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
reason="The goal of these tests are to test the serving components of our feature", | ||
@pytest.mark.skip( | ||
reason="available_gpus is None as InstanceType is not picked up. Need to see if this test case can be added as " | ||
"Notebook tests: /tests/scripts/run-notebook-test.sh", |
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.
Can we remove these instead of just skipping? what is the long term plan for these ? is there any alternative to provisioning GPU
* Skip JS Tune integration test * Revert "Skip JS Tune integration test" This reverts commit 627f394. * Revert "Revert "Skip JS Tune integration test"" * Remove integ test case --------- Co-authored-by: Jonathan Makunga <[email protected]>
* Skip JS Tune integration test * Revert "Skip JS Tune integration test" This reverts commit 627f394. * Revert "Revert "Skip JS Tune integration test"" * Remove integ test case --------- Co-authored-by: Jonathan Makunga <[email protected]>
Issue #, if available:
Description of changes:
Skip Integration test case for JS Tune in local mode.
This test case may be suitable as Notebook test as it requires access to instance GPU count to determine
Tensor parallel degrees.
Testing done:
Merge Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
unique_name_from_base
to create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.