feature: Torch dependency in sagameker-core to be made optional (5457)#5713
feature: Torch dependency in sagameker-core to be made optional (5457)#5713aviruthen wants to merge 2 commits intoaws:masterfrom
Conversation
sagemaker-bot
left a comment
There was a problem hiding this comment.
🤖 AI Code Review
This PR makes the torch dependency optional in sagemaker-core, which is a good change to reduce the installation footprint. However, there are several issues: the serializer's lazy import pattern has a bug where the Tensor reference is stored on self but the import error is raised in __init__, the deserializer already had a try/except but raised a generic Exception (now fixed), and the tests have some issues with variable scoping and could be more robust.
🤖 Iteration #1 — Review Comments AddressedMake torch dependency optional in sagemaker-coreDescriptionThe
Changes
Backward Compatibility NoteRemoving Comments reviewed: 9
|
🤖 Iteration #1 — Review Comments AddressedMake torch dependency optional in sagemaker-coreDescriptionThe
Changes
Backward Compatibility NoteRemoving Note on CI FailuresAll CI failures in the previous iteration were from unrelated integration tests ( Comments reviewed: 17
|
sagemaker-bot
left a comment
There was a problem hiding this comment.
🤖 AI Code Review
This PR makes the torch dependency optional in sagemaker-core, which is a good change to reduce the installation footprint. The implementation is mostly correct, but there are a few issues: the serializer has a bug where self.torch_tensor is never assigned when torch is missing (the exception propagates, so it's fine), the test file needs some improvements for robustness, and the _block_torch helper needs to handle module reload edge cases better.
🤖 Iteration #2 — Review Comments AddressedMake torch dependency optional in sagemaker-coreDescriptionThe
Changes
Breaking Change NoteRemoving Comments reviewed: 25
|
🤖 Iteration #3 — Review Comments AddressedMake torch dependency optional in sagemaker-coreDescriptionThe
Changes
Backward Compatibility NoteRemoving Comments reviewed: 18
|
🤖 Iteration #4 — Review Comments AddressedMake torch dependency optional in sagemaker-coreDescriptionThe
Changes
Backward Compatibility NoteRemoving Comments reviewed: 18
|
Description
The
torch>=1.9.0dependency in sagemaker-core/pyproject.toml is a hard requirement but is only used in narrow, optional scenarios: (1) TorchTensorSerializer/TorchTensorDeserializer which already use lazy imports with try/except, (2) torchrun_driver.py which runs inside SageMaker containers that already have torch, and (3) string-level references in fw_utils.py/image_uris.py that don't import torch. The fix is to move torch from required dependencies to an optional dependency group (e.g., [torch]) in pyproject.toml, and ensure all code paths that use torch handle ImportError gracefully via lazy imports with DeferredError or try/except.Related Issue
Related issue: 5457
Changes Made
sagemaker-core/pyproject.tomlsagemaker-core/src/sagemaker/core/serializers/base.pysagemaker-core/src/sagemaker/core/deserializers/base.pysagemaker-core/tests/unit/test_optional_torch_dependency.pyAI-Generated PR
This PR was automatically generated by the PySDK Issue Agent.
Merge Checklist
prefix: descriptionformat