Fix ModelBuilder networking initialization with core Networking objects#5855
Fix ModelBuilder networking initialization with core Networking objects#5855aryanputta wants to merge 1 commit into
Conversation
Signed-off-by: Aryan <aryansputta@gmail.com>
Mattral
left a comment
There was a problem hiding this comment.
Hello there, one quick thing I want to learn from your PR, thanks for fixing the initialization logic to support both legacy and core Networking objects. One question: if future versions of Networking introduce new attributes beyond vpc_config, do you plan to handle those generically in ModelBuilder or continue patching case‑by‑case? Clarifying this could help maintainers plan for forward compatibility.
After looking through the code again, sagemaker.core.training.configs.Networking currently only exposes the fields that ModelBuilder actually uses here: subnets, security_group_ids, and enable_network_isolation. It does not seem to expose a stable vpc_config attribute, and ModelBuilder is already doing the normalization itself. So I would lean toward keeping this fix narrow. ModelBuilder should support the two shapes it needs today: older objects that have vpc_config, and core networking objects that expose subnets, security_group_ids, and enable_network_isolation. I would avoid copying over arbitrary future attributes unless those fields actually change the deployment request shape. If core Networking eventually adds one canonical method to convert itself into the request format, then I think that would be the cleaner long term path instead of adding open ended attribute introspection inside ModelBuilder. Because of that, I do not think this PR needs another code change right now. The current fix is intentionally scoped: it normalizes the two known shapes without making ModelBuilder more tightly coupled to future Networking fields that it may not even need. |
Summary
Fixes #5827.
Testing
imported ModelBuilder with torch stubbed, instantiated a real Networking object, and verified that ModelBuilder initializes VpcConfig and network isolation without raising
PY
Notes