test: unskip sm-core integ tests#5856
Conversation
lucasjia-aws
force-pushed
the
unskip-integ-tests-core
branch
from
4147d4a to
076b890
Compare
lucasjia-aws
force-pushed
the
unskip-integ-tests-core
branch
from
67d18ca to
24fcdac
Compare
lucasjia-aws
force-pushed
the
unskip-integ-tests-core
branch
from
24fcdac to
b17f7b7
Compare
lucasjia-aws
force-pushed
the
unskip-integ-tests-core
branch
from
c8108c7 to
6aa560b
Compare
lucasjia-aws
force-pushed
the
unskip-integ-tests-core
branch
from
6aa560b to
ff810ee
Compare
lucasjia-aws
force-pushed
the
unskip-integ-tests-core
branch
from
f75ac7e to
24bc6d5
Compare
| from tests.integ.s3_utils import assert_s3_files_exist | ||
|
|
||
| # from tests.integ.kms_utils import get_or_create_kms_key # TODO: provide KMS utils | ||
| from tests.integ.kms_utils import get_or_create_kms_key |
There was a problem hiding this comment.
If you're creating a kms key, it should be cleaned up (ie deleted) after test ends.
There was a problem hiding this comment.
Currently it uses a fixed alias (SageMakerTestKMSKey) and reuses the same key across test runs — it only creates a new key if one doesn't already exist under that alias. This is the same pattern used in
kms_utils.py (which I ported this from), where the key persists in the CI account and is shared across all integ tests that need KMS.
The reason for this design: KMS keys have a mandatory 7-day minimum deletion window (schedule_key_deletion(PendingWindowInDays=7)), so creating and deleting a key per test run isn't practical. The persistent shared key approach avoids accumulating orphaned keys and unnecessary costs.
| # may not use this file except in compliance with the License. A copy of | ||
| # the License is located at | ||
| # | ||
| # http://aws.amazon.com/apache2.0/ |
There was a problem hiding this comment.
Nitpick: Can we rename this file so it indicates this file only to help with integ tests? Current name makes it sound like its a part of sm-core.
Also, can you check if the codebase does not already have util functions to get/create kms keys. Most like there isn't so you're good.
There was a problem hiding this comment.
Good point — I'll rename it to integ_test_kms_helpers.py.
Confirmed there's no existing KMS utility anywhere in sagemaker-core. This was ported from
kms_utils.py which is the canonical implementation used across the old SDK's integ tests. When sagemaker-core was split out, this test helper seems like wasn't carried over.
…ineffective locals() assignment with args dict
…ced_job_setting - Add tests/integ/kms_utils.py with get_or_create_kms_key utility ported from SageMakerHulkPythonSDK, adapted for sagemaker-core import paths - Uncomment kms_utils import in test_decorator.py to fix NameError in s3_kms_key fixture
… fix test bugs
- Add skipif for Python versions without Spark image (only 3.9/3.12 supported)
- Fix Properties set literal to dict: {"spark.app.name": "remote-spark-test"}
- Fix spark.conf.get() to use string key instead of attribute reference
…hub schema additions" This reverts commit 1e12c52.
lucasjia-aws
force-pushed
the
unskip-integ-tests-core
branch
from
eaf955b to
6f6fda9
Compare
3 participants
Description
This PR audits and unskips integration tests in
sagemaker-coreas part of COE AI4 (skipped test audit). Each previously-skipped test was investigated for necessity, root cause of skip, and fixed where possible.Summary of Changes
test_retrieve_image_uri_intelligent_default28c168e3,533f3c6a,b17f7b7fImageRetriever: (1) replaced ineffectivelocals()assignment withargsdict, (2) added missingPYTHON_SDKin config path, (3) used PascalCase (_to_pascal_case) for config key lookup. Updated expected image version from 3.0.0 to 3.2.0.test_advanced_job_settinga1e0e17ekms_utils.pyfrom SageMakerHulkPythonSDK withget_or_create_kms_keyutility, adapted for sagemaker-core import paths. Uncommented the import in test file.test_decorator_with_spark_job6aa560bc@pytest.mark.skipiffor Python != 3.9 or 3.12 -- SageMaker Spark image only available for Python 3.9 and 3.12test_decorator_auto_capture5a914837Tests that remain skipped (with justification)
test_decorator_with_spark_jobsagemaker-spark-processing:3.3-cpu-py39-v1) does not have a workingpipin PATH. SDK auto-injectspip install sagemaker-feature-store-pysparkas a pre-exec command which fails. Requires Spark container image update. Conditionally skipped with@pytest.mark.skipifso it auto-enables when run on py39 with a fixed container.Testing
sagemaker-coreinteg test suite via CodeBuildtest_decorator_with_spark_jobon py39 and py312 to confirm root causetest_advanced_job_setting