Skip to content

Commit dd2bef8

Browse files
committed
chore(serve): trim verbose comments
1 parent 11e6962 commit dd2bef8

3 files changed

Lines changed: 18 additions & 78 deletions

File tree

‎sagemaker-serve/src/sagemaker/serve/bedrock_model_builder.py‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -659,11 +659,8 @@ def _get_checkpoint_uri_from_manifest(self) -> Optional[str]:
659659
if not isinstance(self.model, TrainingJob):
660660
raise ValueError("Model must be a TrainingJob instance for Nova models")
661661

662-
# Nova serverless training jobs (SFTTrainer/RLVRTrainer/DPOTrainer) do
663-
# not populate model_artifacts (there is no model.tar.gz); the manifest
664-
# lives under the job's output_data_config path. This mirrors how
665-
# ModelBuilder._resolve_nova_escrow_uri and the official Nova Studio
666-
# notebook (sm-studio-nova-training-job-sample-notebook.ipynb) locate it.
662+
# Nova serverless training jobs have no model_artifacts; the manifest
663+
# lives under the job's output_data_config path.
667664
output_data_config = getattr(self.model, "output_data_config", None)
668665
s3_output_path = getattr(output_data_config, "s3_output_path", None)
669666
if not s3_output_path:

‎sagemaker-serve/src/sagemaker/serve/model_builder.py‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4700,9 +4700,7 @@ def _resolve_nova_escrow_uri(self) -> str:
47004700
elif isinstance(self.model, ModelTrainer):
47014701
training_job = self.model._latest_training_job
47024702
elif isinstance(self.model, BaseTrainer) and hasattr(self.model, "_latest_training_job"):
4703-
# SFTTrainer / RLVRTrainer / DPOTrainer etc. expose the underlying
4704-
# TrainingJob via _latest_training_job, like _is_model_customization
4705-
# and _fetch_model_package_arn handle them.
4703+
# SFTTrainer / RLVRTrainer / DPOTrainer expose the job via _latest_training_job.
47064704
training_job = self.model._latest_training_job
47074705
else:
47084706
raise ValueError("Nova escrow URI resolution requires a TrainingJob or ModelTrainer")

‎sagemaker-serve/tests/integ/test_nova_model_customization_deployment.py‎

Lines changed: 15 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,8 @@
1212
# language governing permissions and limitations under the License.
1313
"""Integration tests for Nova model customization deployment.
1414
15-
These tests are the Nova counterpart of test_model_customization_deployment.py
16-
and cover two deployment targets for a fine-tuned Nova model:
17-
- SageMaker endpoints via ModelBuilder (TestModelCustomization* classes).
18-
- Amazon Bedrock custom models via BedrockModelBuilder (TestNovaBedrockDeployment).
19-
20-
They run against the dedicated Nova test account in us-east-1 (784379639078)
21-
and are marked with ``us_east_1`` so the PR check integ-tests-us-east-1 job
22-
picks them up (they are intentionally not marked ``gpu_intensive``, so they do
23-
not run in the scheduled GPU workflow).
15+
Covers deploying a fine-tuned Nova model to SageMaker endpoints (via
16+
ModelBuilder) and to Amazon Bedrock custom models (via BedrockModelBuilder).
2417
"""
2518
from __future__ import absolute_import
2619

@@ -38,29 +31,19 @@
3831

3932
from sagemaker.core.helper.session_helper import Session
4033

41-
# This test relies on resources in a specific region (Nova test account)
34+
# This test relies on resources in a specific region
4235
AWS_REGION = "us-east-1"
4336
os.environ.setdefault("AWS_DEFAULT_REGION", AWS_REGION)
4437

4538
# Model package group shared with the Nova SFT/RLVR trainer integ tests.
46-
# Training jobs in those tests register their output here.
4739
MODEL_PACKAGE_GROUP = "sdk-test-finetuned-models"
4840

49-
# Nova base model id (matches the existing Nova trainer/evaluator integ tests).
5041
NOVA_MODEL_ID = "nova-textgeneration-lite-v2"
51-
52-
# Nova deployment instance type (matches test_sft_trainer_nova_workflow setup).
5342
NOVA_INSTANCE_TYPE = "ml.g6.48xlarge"
5443

5544

5645
def _deploy_or_skip_on_capacity(model_builder, **deploy_kwargs):
57-
"""Deploy via ModelBuilder, skipping the test on transient capacity errors.
58-
59-
GPU instance types like ml.g6.48xlarge can hit InsufficientInstanceCapacity
60-
(a transient, region-wide availability issue, not a quota or code problem),
61-
which would otherwise fail the deploy with a FailedStatusError. Skip rather
62-
than fail in that case so CI stays green on capacity fluctuations.
63-
"""
46+
"""Deploy via ModelBuilder, skipping on transient InsufficientInstanceCapacity."""
6447
try:
6548
return model_builder.deploy(**deploy_kwargs)
6649
except Exception as e:
@@ -73,9 +56,6 @@ def _deploy_or_skip_on_capacity(model_builder, **deploy_kwargs):
7356
def _latest_model_package_arn(region=AWS_REGION):
7457
"""Return the ARN of the most recently created Completed model package in
7558
the Nova model package group, or None if the group has no usable package.
76-
77-
Mirrors the dynamic lookup used by test_benchmark_evaluation_nova_model so
78-
these tests stay decoupled from any specific model package version.
7959
"""
8060
sm_client = boto3.client("sagemaker", region_name=region)
8161
packages = sm_client.list_model_packages(
@@ -87,7 +67,6 @@ def _latest_model_package_arn(region=AWS_REGION):
8767
)
8868
summaries = packages.get("ModelPackageSummaryList", [])
8969
if not summaries:
90-
# Fall back to any status if no Approved packages exist.
9170
packages = sm_client.list_model_packages(
9271
ModelPackageGroupName=MODEL_PACKAGE_GROUP,
9372
SortBy="CreationTime",
@@ -114,13 +93,8 @@ def training_job_name():
11493
"""Most recent completed Nova SFT training job whose output model package
11594
still exists.
11695
117-
The gpu-integ-tests-us-east-1 scheduled workflow runs
118-
test_sft_trainer_nova_workflow every few hours, each producing a fresh
119-
sft-nova-integ-* training job whose output is registered to
120-
sdk-test-finetuned-models. We discover the latest usable one at runtime
121-
rather than hardcoding a name: hardcoded jobs eventually get cleaned up and
122-
their output model package is deleted, leaving a dangling ARN (the previous
123-
reusable job pointed at the now-deleted sdk-test-nova-finetuned-models).
96+
Discovered at runtime rather than hardcoded so the test does not depend on a
97+
job name that may be cleaned up.
12498
"""
12599
sm_client = boto3.client("sagemaker", region_name=AWS_REGION)
126100
jobs = sm_client.list_training_jobs(
@@ -138,7 +112,6 @@ def training_job_name():
138112
if not mp_arn:
139113
continue
140114
try:
141-
# Confirm the registered model package still exists.
142115
sm_client.describe_model_package(ModelPackageName=mp_arn)
143116
return name
144117
except sm_client.exceptions.ClientError:
@@ -235,8 +208,7 @@ def test_deploy_from_training_job(self, training_job_name, endpoint_name, cleanu
235208
assert endpoint.endpoint_arn is not None
236209
assert endpoint.endpoint_status == "InService"
237210

238-
# Invoke verification
239-
time.sleep(10) # brief buffer for IC readiness
211+
time.sleep(10) # brief buffer for inference component readiness
240212

241213
invoke_response = endpoint.invoke(
242214
body=json.dumps({
@@ -250,7 +222,6 @@ def test_deploy_from_training_job(self, training_job_name, endpoint_name, cleanu
250222

251223
response_body = json.loads(invoke_response.body.read())
252224

253-
# Validate response structure
254225
assert response_body is not None, f"Empty response from invoke on {endpoint_name}"
255226
assert isinstance(response_body, dict)
256227

@@ -267,12 +238,8 @@ def test_fetch_endpoint_names_for_base_model(self, training_job_name, sagemaker_
267238
class TestModelCustomizationFromModelPackage:
268239
"""Test Nova model customization deployment via the registered model package.
269240
270-
Nova model artifacts live in an escrow bucket whose location is only
271-
resolvable from the training job's manifest.json (see
272-
ModelBuilder._resolve_nova_escrow_uri, which requires a TrainingJob or
273-
ModelTrainer). Deploying a Nova model directly from a ModelPackage is
274-
therefore not supported, so these tests drive the supported path: build /
275-
deploy from the TrainingJob and validate the model package it registered.
241+
A fine-tuned Nova model is built and deployed from its TrainingJob; the
242+
registered model package is validated along the way.
276243
"""
277244

278245
def test_build_from_model_package(self, training_job_name, sagemaker_session):
@@ -288,7 +255,6 @@ def test_build_from_model_package(self, training_job_name, sagemaker_session):
288255

289256
assert model is not None
290257
assert model.model_arn is not None
291-
# The training job should have registered a model package.
292258
assert model_builder._fetch_model_package_arn() is not None
293259

294260
def test_deploy_from_model_package(self, training_job_name, endpoint_name, cleanup_endpoints, sagemaker_session):
@@ -363,8 +329,7 @@ def test_fetch_model_package_arn(self, training_job_name, sagemaker_session):
363329
class TestTrainerIntegration:
364330
"""Test ModelBuilder integration with Nova SFTTrainer and RLVRTrainer.
365331
366-
Nova does not have a DPO recipe in SageMakerPublicHub (only SFT/RLVR/CPT/MTRL),
367-
so the DPO build test from the open-weights suite is replaced with RLVR here.
332+
Nova has no DPO recipe, so RLVR is used in place of the open-weights DPO test.
368333
"""
369334

370335
def test_sft_trainer_build(self, training_job_name, sagemaker_session):
@@ -424,16 +389,7 @@ def test_rlvr_trainer_build(self, training_job_name, sagemaker_session):
424389

425390
@pytest.mark.us_east_1
426391
class TestNovaBedrockDeployment:
427-
"""Test deploying a fine-tuned Nova model to Amazon Bedrock.
428-
429-
Unlike open-weight (OSS) models, which Bedrock serves via a Custom Model
430-
Import job (create_model_import_job), Nova models are deployed through
431-
Bedrock custom models: BedrockModelBuilder.deploy() detects the Nova model
432-
and calls create_custom_model + create_custom_model_deployment, polling each
433-
resource to Active before returning.
434-
435-
These tests run against the Nova test account in us-east-1 (784379639078).
436-
"""
392+
"""Test deploying a fine-tuned Nova model to Amazon Bedrock as a custom model."""
437393

438394
@pytest.fixture(scope="class")
439395
def role_arn(self):
@@ -448,19 +404,15 @@ def bedrock_client(self):
448404

449405
@pytest.fixture(scope="class")
450406
def bedrock_runtime(self):
451-
"""Create a Bedrock runtime client with retries for cold custom models."""
407+
"""Bedrock runtime client with retries for not-yet-ready custom models."""
452408
from botocore.config import Config
453409
config = Config(retries={"total_max_attempts": 10, "mode": "standard"})
454410
return boto3.client("bedrock-runtime", region_name=AWS_REGION, config=config)
455411

456412
@pytest.fixture(scope="class")
457413
def deployed_nova_model(self, training_job_name, role_arn, bedrock_client):
458-
"""Deploy a Nova model to Bedrock and yield deployment details.
459-
460-
Nova artifacts live in an escrow bucket resolved from the training job's
461-
manifest.json, so BedrockModelBuilder is driven from the TrainingJob
462-
(deploying from a ModelPackage is not supported for non-RMP Nova models).
463-
Cleans up the custom model and its deployment after the class completes.
414+
"""Deploy a Nova model to Bedrock from its TrainingJob and yield the
415+
deployment details, cleaning up the custom model and deployment after.
464416
"""
465417
from sagemaker.core.resources import TrainingJob
466418
from sagemaker.serve.bedrock_model_builder import BedrockModelBuilder
@@ -485,7 +437,6 @@ def deployed_nova_model(self, training_job_name, role_arn, bedrock_client):
485437
deployment_arn = response.get("customModelDeploymentArn")
486438
assert deployment_arn is not None, f"No deployment ARN in response: {response}"
487439

488-
# Resolve the underlying custom model ARN for cleanup.
489440
deployment = bedrock_client.get_custom_model_deployment(
490441
customModelDeploymentIdentifier=deployment_arn
491442
)
@@ -499,7 +450,6 @@ def deployed_nova_model(self, training_job_name, role_arn, bedrock_client):
499450
except Exception as e:
500451
pytest.fail(f"Nova Bedrock deployment failed: {e}")
501452
finally:
502-
# Cleanup deployment first, then the custom model.
503453
if deployment_arn:
504454
try:
505455
bedrock_client.delete_custom_model_deployment(
@@ -525,11 +475,7 @@ def test_nova_bedrock_deployment_active(self, deployed_nova_model, bedrock_clien
525475

526476
@pytest.mark.slow
527477
def test_nova_bedrock_invoke(self, deployed_nova_model, bedrock_runtime):
528-
"""Invoke the deployed Nova model on Bedrock end-to-end.
529-
530-
The runtime client is configured with retries to tolerate the brief
531-
window where a freshly-deployed custom model is not yet servable.
532-
"""
478+
"""Invoke the deployed Nova model on Bedrock end-to-end."""
533479
deployment_arn = deployed_nova_model["deployment_arn"]
534480

535481
response = bedrock_runtime.invoke_model(
@@ -547,7 +493,6 @@ def test_nova_bedrock_invoke(self, deployed_nova_model, bedrock_runtime):
547493

548494
result = json.loads(response["body"].read().decode())
549495

550-
# Validate response structure (Nova returns output.message.content[].text).
551496
assert result is not None, "Empty response from Bedrock invoke"
552497
assert isinstance(result, dict)
553498
text = result["output"]["message"]["content"][0]["text"]

0 commit comments

Comments
 (0)