Skip to content

fix(train): correct Networking field names in ModelTrainer intelligent defaults - #5866

Open
mandipat wants to merge 2 commits into
aws:masterfrom
mandipat:fix/networking-default-enable-network-isolation
Open

fix(train): correct Networking field names in ModelTrainer intelligent defaults#5866
mandipat wants to merge 2 commits into
aws:masterfrom
mandipat:fix/networking-default-enable-network-isolation

Conversation

@mandipat

Copy link
Copy Markdown

Summary

Fixes two bugs in _populate_intelligent_defaults_from_training_job_space() in model_trainer.py that prevent ModelTrainer from working when sagemaker_config has a VpcConfig set. This makes SageMaker distribution 4.0 unusable out of the box.

Bug 1 — Wrong field name passed to Networking constructor

default_enable_network_isolation does not exist as a field in sagemaker-core 2.x. The correct field name is enable_network_isolation. This caused a Pydantic ValidationError on every model_trainer.train() call when VPC config was present in sagemaker_config.

# BEFORE (broken)
self.networking = Networking(
    default_enable_network_isolation=default_enable_network_isolation,  # field does not exist
    ...
)

# AFTER (fixed)
self.networking = Networking(
    enable_network_isolation=default_enable_network_isolation,
    ...
)

Bug 2 — security_group_ids silently assigned wrong value

When updating an existing Networking object, security_group_ids was mistakenly assigned the value from TRAINING_JOB_SUBNETS_PATH instead of TRAINING_JOB_SECURITY_GROUP_IDS_PATH.

# BEFORE (broken)
if self.networking.security_group_ids is None:
    self.networking.subnets = self.config_mgr.resolve_value_from_config(  # wrong attribute + wrong path
        config_path=TRAINING_JOB_SUBNETS_PATH
    )

# AFTER (fixed)
if self.networking.security_group_ids is None:
    self.networking.security_group_ids = self.config_mgr.resolve_value_from_config(
        config_path=TRAINING_JOB_SECURITY_GROUP_IDS_PATH
    )

Testing

Reproduced and verified the fix using a clean venv with sagemaker-core==2.10.0 and sagemaker-train==1.10.0 (exact versions shipped in SageMaker distribution 4.0).

from sagemaker.core.training.configs import Networking

# Bug 1 confirmed
try:
    Networking(default_enable_network_isolation=None)  # ValidationError
except Exception as e:
    print(f"BUG: {e}")

# Fix confirmed
Networking(enable_network_isolation=False)  # Works

Closes #5766

…t defaults

Two bugs in _populate_intelligent_defaults_from_training_job_space():

1. Networking constructor was passed `default_enable_network_isolation`
   which does not exist in sagemaker-core 2.x. The correct field name
   is `enable_network_isolation`. This caused a Pydantic ValidationError
   when sagemaker_config had a VpcConfig value set, making ModelTrainer
   unusable in SageMaker distribution 4.0 out of the box.

2. When updating an existing Networking object, `security_group_ids` was
   mistakenly assigned the subnets value from TRAINING_JOB_SUBNETS_PATH
   instead of the correct value from TRAINING_JOB_SECURITY_GROUP_IDS_PATH.

Fixes aws#5766
@jam-jee
jam-jee force-pushed the fix/networking-default-enable-network-isolation branch from 3e79bac to 203ddf3 Compare July 8, 2026 03:58
@jam-jee
jam-jee temporarily deployed to manual-approval July 8, 2026 03:58 — with GitHub Actions Inactive
@jam-jee
jam-jee temporarily deployed to manual-approval July 8, 2026 03:58 — with GitHub Actions Inactive
@mandipat
mandipat temporarily deployed to manual-approval July 8, 2026 14:40 — with GitHub Actions Inactive
@mandipat
mandipat temporarily deployed to manual-approval July 8, 2026 14:40 — with GitHub Actions Inactive
@jam-jee

jam-jee commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Failed integ tests are not related to the changes made in this PR.

jam-jee
jam-jee previously approved these changes Jul 10, 2026
@jam-jee
jam-jee self-requested a review July 10, 2026 05:35

@jam-jee jam-jee left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add/update relevant unit tests for this change.

@jam-jee
jam-jee self-requested a review July 13, 2026 21:19
@jam-jee
jam-jee dismissed their stale review July 14, 2026 05:10

added comments to add relevant tests

@admivsn

admivsn commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Hey, please let me know how we can get this or the original PR (#5767) merged to fix this issue. Happy to support

mohamedzeidan2021 added a commit that referenced this pull request Jul 21, 2026
…t defaults (#6064)

Fixes two bugs in _populate_intelligent_defaults_from_training_job_space()
in model_trainer.py that prevent ModelTrainer from working when
sagemaker_config has a VpcConfig set.

Bug 1: Networking() was passed default_enable_network_isolation, which is
not a field in sagemaker-core 2.x. The correct field is
enable_network_isolation. This raised a pydantic ValidationError on every
train() call when VPC config was present.

Bug 2: When updating an existing Networking object, security_group_ids was
mistakenly assigned the subnets value from TRAINING_JOB_SUBNETS_PATH instead
of security_group_ids from TRAINING_JOB_SECURITY_GROUP_IDS_PATH.

Adds unit tests covering both bug paths (fresh Networking construction and
updating an existing Networking object).

Fix originally from #5866; unit tests added here.

Co-authored-by: Mohamed Zeidan <zeidmo@amazon.com>
@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

merged in your PR with unit tests here: #6064

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.

sagemaker_config related Networking bugs in ModelTrainer

4 participants