Skip to content

Commit e4d6cc8

Browse files
authored
fix(instance-preferences): run integ tests in CI, reject managed spot client-side, stabilize IntPipeVar codegen (#6250)
* fix(instance-preferences): reject managed spot client-side; make IntPipeVar codegen-stable The documentation lists managed spot training among the settings that are mutually exclusive with instance_preferences, but the client-side validator only checked instance_type, instance_groups and instance_placement_config, so the spot conflict surfaced as a service error. Both validator copies now reject an explicit enable_managed_spot_training=True; False and unset remain valid. The IntPipeVar annotation on the five instance-preferences count members was a hand edit that generate_shapes could not reproduce: only ResourceConfig.InstanceCount was registered in PIPE_VAR_OVERRIDES, so the next regeneration would have narrowed the others back to Optional[int]. All five are registered, and a unit test asserts the extractor emits IntPipeVar for each so a dropped override fails loudly. * test(instance-preferences): run integ tests in the standard CI account Both integ tests were gated on account-specific environment variables that no CI configuration sets, so every CI run skipped them and the suite passed without exercising the feature. Training coverage: - tests/integ/train/shallow: two accepted cases (uniform and per-preference counts; submit, Describe echo, stop) and four rejected cases (instance_type, managed spot and duplicate types client-side; the five-entry cap server-side). Runs on every pull request. - tests/integ/train/test_instance_preferences.py: a CPU-only end-to-end test that submits a job with a preference list and follows it until the service selects a winner and the job completes on it, asserting the winner is one of the submitted types and the top-level type stays unset. It stops the job in a finally block so it never runs to max_runtime on shared quota. The processing test uses the suite's SageMakerRole and resolves its image through image_uris, as the other sagemaker-core integration tests do, and now stops the job in a finally block once the winner is observed. Candidate types remain overridable via *_TEST_INSTANCE_TYPES for accounts whose default types lack quota; no account-specific values remain. Verified in us-west-2: all six shallow cases pass, the training e2e runs to Completed on a selected winner, and the processing test passes with a resolved winner.
1 parent 3d58dc9 commit e4d6cc8

8 files changed

Lines changed: 427 additions & 188 deletions

File tree

‎sagemaker-core/src/sagemaker/core/modules/utils.py‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
# ANY KIND, either express or implied. See the License for the specific
1212
# language governing permissions and limitations under the License.
1313
"""Utils module."""
14+
1415
from __future__ import absolute_import
1516

1617
import os
@@ -200,7 +201,7 @@ def validate_instance_preferences(compute) -> None:
200201
201202
- instance_preferences is mutually exclusive with the classic
202203
single-cluster fields instance_type / instance_groups /
203-
instance_placement_config.
204+
instance_placement_config, and with managed spot training.
204205
- Instance types must not repeat across preferences.
205206
- Count mode: exactly one of the top-level instance_count (applies to
206207
whichever preference wins) with no per-preference instance_count, or an
@@ -231,6 +232,12 @@ def _value(obj, field):
231232
f"instance_preferences is mutually exclusive with {field}; "
232233
"specify either a single fixed cluster or instance_preferences, not both."
233234
)
235+
# Spot is a bool: only an explicit True conflicts (False/None is the default).
236+
if _value(compute, "enable_managed_spot_training") is True:
237+
raise ValueError(
238+
"instance_preferences is mutually exclusive with managed spot training "
239+
"(enable_managed_spot_training=True)."
240+
)
234241

235242
instance_types = [_value(p, "instance_type") for p in preferences]
236243
duplicates = sorted(

‎sagemaker-core/src/sagemaker/core/tools/constants.py‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
# ANY KIND, either express or implied. See the License for the specific
1212
# language governing permissions and limitations under the License.
1313
"""Constants used in the code_generator modules."""
14+
1415
import os
1516

1617
CLASS_METHODS = set(["create", "add", "register", "import", "list", "get"])
@@ -134,5 +135,16 @@
134135
"InstanceCount": "IntPipeVar",
135136
"VolumeSizeInGB": "IntPipeVar",
136137
"KeepAlivePeriodInSeconds": "IntPipeVar",
138+
"SelectedInstanceCount": "IntPipeVar",
139+
},
140+
"InstancePreference": {
141+
"InstanceCount": "IntPipeVar",
142+
},
143+
"ProcessingClusterConfig": {
144+
"InstanceCount": "IntPipeVar",
145+
"SelectedInstanceCount": "IntPipeVar",
146+
},
147+
"ProcessingInstancePreference": {
148+
"InstanceCount": "IntPipeVar",
137149
},
138150
}

‎sagemaker-core/src/sagemaker/core/training/utils.py‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ def validate_instance_preferences(compute) -> None:
264264
265265
- instance_preferences is mutually exclusive with the classic
266266
single-cluster fields instance_type / instance_groups /
267-
instance_placement_config.
267+
instance_placement_config, and with managed spot training.
268268
- Instance types must not repeat across preferences.
269269
- Count mode: exactly one of the top-level instance_count (applies to
270270
whichever preference wins) with no per-preference instance_count, or an
@@ -295,6 +295,12 @@ def _value(obj, field):
295295
f"instance_preferences is mutually exclusive with {field}; "
296296
"specify either a single fixed cluster or instance_preferences, not both."
297297
)
298+
# Spot is a bool: only an explicit True conflicts (False/None is the default).
299+
if _value(compute, "enable_managed_spot_training") is True:
300+
raise ValueError(
301+
"instance_preferences is mutually exclusive with managed spot training "
302+
"(enable_managed_spot_training=True)."
303+
)
298304

299305
instance_types = [_value(p, "instance_type") for p in preferences]
300306
duplicates = sorted(

‎sagemaker-core/tests/integ/processing/test_instance_preferences_processing.py‎

Lines changed: 71 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,11 @@
2424
``SelectedInstanceCount`` report the resolved winner, which must be one of
2525
the submitted preferences.
2626
27-
Configuration (environment variables; SKIPPED when unset):
28-
29-
- ``PROCESSING_INSTANCE_PREFERENCES_TEST_ROLE_ARN`` - execution role ARN
30-
- ``PROCESSING_INSTANCE_PREFERENCES_TEST_IMAGE_URI`` - processing image
31-
- ``PROCESSING_INSTANCE_PREFERENCES_TEST_INSTANCE_TYPES`` - optional comma-separated
32-
candidate types (default: ml.m5.xlarge, ml.m5.large)
33-
- ``SAGEMAKER_ENDPOINT`` - optional endpoint
34-
override.
27+
Runs in the standard integration-test account: the execution role is the
28+
suite's ``SageMakerRole`` and the image is resolved through ``image_uris``, the
29+
same way the other ``sagemaker-core`` integration tests obtain theirs.
30+
``PROCESSING_INSTANCE_PREFERENCES_TEST_INSTANCE_TYPES`` (comma-separated)
31+
overrides the candidate types for accounts where the defaults lack quota.
3532
"""
3633

3734
from __future__ import absolute_import
@@ -40,11 +37,12 @@
4037
import time
4138
import uuid
4239

43-
import pytest
40+
import boto3
4441

45-
ROLE_ARN = os.environ.get("PROCESSING_INSTANCE_PREFERENCES_TEST_ROLE_ARN")
46-
IMAGE_URI = os.environ.get("PROCESSING_INSTANCE_PREFERENCES_TEST_IMAGE_URI")
47-
ENDPOINT = os.environ.get("SAGEMAKER_ENDPOINT")
42+
from sagemaker.core import image_uris
43+
44+
ROLE = "SageMakerRole"
45+
REGION = os.environ.get("AWS_REGION", os.environ.get("AWS_DEFAULT_REGION", "us-west-2"))
4846

4947
PREFERENCE_TYPES = [
5048
t.strip()
@@ -55,36 +53,40 @@
5553
]
5654
WAIT_TIMEOUT_SECONDS = 30 * 60
5755
POLL_SECONDS = 30
58-
59-
pytestmark = pytest.mark.skipif(
60-
not (ROLE_ARN and IMAGE_URI),
61-
reason=(
62-
"Processing Instance Preferences integ test requires "
63-
"PROCESSING_INSTANCE_PREFERENCES_TEST_ROLE_ARN and "
64-
"PROCESSING_INSTANCE_PREFERENCES_TEST_IMAGE_URI"
65-
),
66-
)
67-
68-
69-
def _sagemaker_client():
70-
"""Build a boto3 SageMaker client, honoring SAGEMAKER_ENDPOINT."""
71-
import boto3
72-
73-
region = os.environ.get("AWS_REGION", "us-west-2")
74-
return boto3.client("sagemaker", region_name=region, endpoint_url=ENDPOINT)
56+
TERMINAL = ("Completed", "Failed", "Stopped")
57+
58+
59+
def _stop_quietly(client, job_name):
60+
"""The job has done its part once a winner is selected; do not leave it
61+
running to max_runtime on shared quota."""
62+
try:
63+
if (
64+
client.describe_processing_job(ProcessingJobName=job_name)["ProcessingJobStatus"]
65+
in TERMINAL
66+
):
67+
return
68+
client.stop_processing_job(ProcessingJobName=job_name)
69+
except Exception: # pylint: disable=broad-except
70+
pass
71+
72+
73+
def _processing_image():
74+
return image_uris.retrieve(
75+
"sklearn", REGION, version="1.2-1", py_version="py3", instance_type=PREFERENCE_TYPES[0]
76+
)
7577

7678

7779
def test_processor_instance_preferences_e2e():
7880
"""Create a real processing job with InstancePreferences and verify the winner."""
7981
from sagemaker.core.helper.session_helper import Session
8082
from sagemaker.core.processing import Processor
8183

82-
client = _sagemaker_client()
84+
client = boto3.client("sagemaker", region_name=REGION)
8385
session = Session(sagemaker_client=client)
8486

8587
processor = Processor(
86-
role=ROLE_ARN,
87-
image_uri=IMAGE_URI,
88+
role=ROLE,
89+
image_uri=_processing_image(),
8890
instance_count=1,
8991
instance_preferences=[{"InstanceType": t} for t in PREFERENCE_TYPES],
9092
volume_size_in_gb=30,
@@ -95,39 +97,43 @@ def test_processor_instance_preferences_e2e():
9597
job_name = f"instance-prefs-proc-integ-{uuid.uuid4().hex[:8]}"
9698
processor.run(wait=False, logs=False, job_name=job_name)
9799

98-
# --- Create accepted; Describe echoes the request contract -------------
99-
described = client.describe_processing_job(ProcessingJobName=job_name)
100-
cluster_config = described["ProcessingResources"]["ClusterConfig"]
101-
assert [p["InstanceType"] for p in cluster_config.get("InstancePreferences", [])] == (
102-
PREFERENCE_TYPES
103-
), f"Describe did not echo InstancePreferences: {cluster_config}"
104-
# The customer never set the top-level InstanceType; it must not come
105-
# back populated on Describe.
106-
assert "InstanceType" not in cluster_config, cluster_config
107-
108-
# --- Wait for a terminal-or-resolved state ------------------------------
109-
deadline = time.time() + WAIT_TIMEOUT_SECONDS
110-
status = described["ProcessingJobStatus"]
111-
while time.time() < deadline:
100+
try:
101+
102+
# --- Create accepted; Describe echoes the request contract -------------
112103
described = client.describe_processing_job(ProcessingJobName=job_name)
104+
cluster_config = described["ProcessingResources"]["ClusterConfig"]
105+
assert [p["InstanceType"] for p in cluster_config.get("InstancePreferences", [])] == (
106+
PREFERENCE_TYPES
107+
), f"Describe did not echo InstancePreferences: {cluster_config}"
108+
# The customer never set the top-level InstanceType; it must not come
109+
# back populated on Describe.
110+
assert "InstanceType" not in cluster_config, cluster_config
111+
112+
# --- Wait for a terminal-or-resolved state ------------------------------
113+
deadline = time.time() + WAIT_TIMEOUT_SECONDS
113114
status = described["ProcessingJobStatus"]
115+
while time.time() < deadline:
116+
described = client.describe_processing_job(ProcessingJobName=job_name)
117+
status = described["ProcessingJobStatus"]
118+
cluster_config = described["ProcessingResources"]["ClusterConfig"]
119+
if status in TERMINAL:
120+
break
121+
if status == "InProgress" and cluster_config.get("SelectedInstanceType"):
122+
break
123+
time.sleep(POLL_SECONDS)
124+
125+
failure_reason = described.get("FailureReason", "")
126+
127+
# --- Full contract: resolved winner is surfaced and is a submitted pref -
114128
cluster_config = described["ProcessingResources"]["ClusterConfig"]
115-
if status in ("Completed", "Failed", "Stopped"):
116-
break
117-
if status == "InProgress" and cluster_config.get("SelectedInstanceType"):
118-
break
119-
time.sleep(POLL_SECONDS)
120-
121-
failure_reason = described.get("FailureReason", "")
122-
123-
# --- Full contract: resolved winner is surfaced and is a submitted pref -
124-
cluster_config = described["ProcessingResources"]["ClusterConfig"]
125-
selected_type = cluster_config.get("SelectedInstanceType")
126-
selected_count = cluster_config.get("SelectedInstanceCount")
127-
assert selected_type in PREFERENCE_TYPES, (
128-
f"SelectedInstanceType={selected_type!r} not among submitted preferences "
129-
f"{PREFERENCE_TYPES} (job={job_name}, status={status}, "
130-
f"failure={failure_reason!r})"
131-
)
132-
assert selected_count == 1
133-
assert "InstanceType" not in cluster_config, cluster_config
129+
selected_type = cluster_config.get("SelectedInstanceType")
130+
selected_count = cluster_config.get("SelectedInstanceCount")
131+
assert selected_type in PREFERENCE_TYPES, (
132+
f"SelectedInstanceType={selected_type!r} not among submitted preferences "
133+
f"{PREFERENCE_TYPES} (job={job_name}, status={status}, "
134+
f"failure={failure_reason!r})"
135+
)
136+
assert selected_count == 1
137+
assert "InstanceType" not in cluster_config, cluster_config
138+
finally:
139+
_stop_quietly(client, job_name)

‎sagemaker-core/tests/unit/test_compute_configs.py‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
"""Unit tests for Compute and HyperPodCompute config classes."""
2+
23
import pytest
34
from sagemaker.core.training.configs import Compute, HyperPodCompute
45

@@ -169,6 +170,26 @@ def test_instance_type_rejected_with_preferences(self, compute_cls):
169170
instance_preferences=[InstancePreference(instance_type="ml.m5.xlarge")],
170171
)
171172

173+
def test_managed_spot_rejected_with_preferences(self, compute_cls):
174+
from sagemaker.core.shapes.shapes import InstancePreference
175+
176+
with pytest.raises(ValueError, match="mutually exclusive with managed spot training"):
177+
compute_cls(
178+
instance_count=1,
179+
enable_managed_spot_training=True,
180+
instance_preferences=[InstancePreference(instance_type="ml.m5.xlarge")],
181+
)
182+
183+
def test_managed_spot_false_allowed_with_preferences(self, compute_cls):
184+
from sagemaker.core.shapes.shapes import InstancePreference
185+
186+
compute = compute_cls(
187+
instance_count=1,
188+
enable_managed_spot_training=False,
189+
instance_preferences=[InstancePreference(instance_type="ml.m5.xlarge")],
190+
)
191+
assert compute.enable_managed_spot_training is False
192+
172193
def test_uniform_count_rejected_with_per_preference_counts(self, compute_cls):
173194
from sagemaker.core.shapes.shapes import InstancePreference
174195

@@ -239,9 +260,7 @@ def test_whole_job_plan_allowed_without_per_preference_plans(self, compute_cls):
239260

240261
compute_cls(
241262
instance_count=1,
242-
training_plan_arn=(
243-
"arn:aws:sagemaker:us-west-2:111122223333:training-plan/whole-job"
244-
),
263+
training_plan_arn=("arn:aws:sagemaker:us-west-2:111122223333:training-plan/whole-job"),
245264
instance_preferences=[
246265
InstancePreference(instance_type="ml.p5.48xlarge"),
247266
InstancePreference(instance_type="ml.p4d.24xlarge"),
@@ -260,9 +279,7 @@ def test_modules_compute_instance_preferences_round_trip(self):
260279
InstancePreference(instance_type="ml.p5.48xlarge"),
261280
InstancePreference(instance_type="ml.p4d.24xlarge"),
262281
]
263-
rc = ModulesCompute(
264-
instance_preferences=prefs, instance_count=2
265-
)._to_resource_config()
282+
rc = ModulesCompute(instance_preferences=prefs, instance_count=2)._to_resource_config()
266283
assert [p.instance_type for p in rc.instance_preferences] == [
267284
"ml.p5.48xlarge",
268285
"ml.p4d.24xlarge",
@@ -271,9 +288,7 @@ def test_modules_compute_instance_preferences_round_trip(self):
271288
def test_modules_compute_single_type_still_works(self):
272289
from sagemaker.core.modules.configs import Compute as ModulesCompute
273290

274-
rc = ModulesCompute(
275-
instance_type="ml.m5.xlarge", instance_count=1
276-
)._to_resource_config()
291+
rc = ModulesCompute(instance_type="ml.m5.xlarge", instance_count=1)._to_resource_config()
277292
assert rc.instance_type == "ml.m5.xlarge"
278293

279294

‎sagemaker-core/tests/unit/tools/test_shapes_extractor.py‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
# ANY KIND, either express or implied. See the License for the specific
1212
# language governing permissions and limitations under the License.
1313
"""Unit tests for sagemaker.core.tools.shapes_extractor module."""
14+
1415
from __future__ import absolute_import
1516

1617
import pytest
@@ -343,3 +344,42 @@ def test_get_required_members_none(self, mock_reformat):
343344
result = extractor.get_required_members("TestShape")
344345

345346
assert len(result) == 0
347+
348+
349+
class TestInstancePreferencesPipeVarOverrides:
350+
"""The IntPipeVar annotations on instance-preferences count members come from
351+
PIPE_VAR_OVERRIDES, not the service model. If an override is dropped, codegen
352+
silently narrows the member back to int and pipeline variables stop being
353+
accepted -- so assert the generated type directly."""
354+
355+
@pytest.fixture
356+
def extractor(self, tmp_path):
357+
# Constructing the extractor regenerates shape_dag.py; point that write
358+
# at a temp file so the unit test leaves the checked-in file alone.
359+
with (
360+
patch("sagemaker.core.tools.shapes_extractor.reformat_file_with_black"),
361+
patch(
362+
"sagemaker.core.tools.shapes_extractor.SHAPE_DAG_FILE_PATH",
363+
str(tmp_path / "shape_dag.py"),
364+
),
365+
):
366+
return ShapesExtractor()
367+
368+
@pytest.mark.parametrize(
369+
"shape, member",
370+
[
371+
("ResourceConfig", "instance_count"),
372+
("ResourceConfig", "selected_instance_count"),
373+
("InstancePreference", "instance_count"),
374+
("ProcessingClusterConfig", "instance_count"),
375+
("ProcessingClusterConfig", "selected_instance_count"),
376+
("ProcessingInstancePreference", "instance_count"),
377+
],
378+
)
379+
def test_count_members_generate_as_int_pipe_var(self, extractor, shape, member):
380+
members = extractor.generate_shape_members(shape)
381+
assert member in members, f"{shape}.{member} missing from generated members"
382+
assert "IntPipeVar" in members[member], (
383+
f"{shape}.{member} generated as {members[member]!r}; "
384+
"expected IntPipeVar via PIPE_VAR_OVERRIDES"
385+
)

0 commit comments

Comments
 (0)