aws / aws/sagemaker-training-toolkit

Remove magic strings for attributes like instance type

Open
#164 0 comments 1 reaction 0 assignees View on GitHub
Dominant language
Python
Stars
530
Forks
140
Avg merge
1h 12m
Merged PRs (30d)
2

Description

**Describe the feature you'd like**
The Sagemaker training toolkit and Python SDK use string literals for attribute values in many places. For instance, SMDDP is supported on P3, P3dn and P4D instances at the moment, and these are used as strings in tests and validation.
Code references:
1. [Hardcoding default parameter values](https://github.com/aws/sagemaker-training-toolkit/blob/885059ffdbeea99ea8e9372b030638081f634c13/src/sagemaker_training/mpi.py#L217)
2. [Test parameters](https://github.com/aws/sagemaker-training-toolkit/blob/a8d0f7f8d8e865602d8fcf6939639188b83f9c41/test/unit/test_smdataparallel.py#L194)
3. [Comments](https://github.com/aws/sagemaker-training-toolkit/blob/fc63b99e20c6c5797558a8d24d347866bbea65ec/src/sagemaker_training/pytorch_xla.py#L81)

If we want to deprecate support for an instance type, or add new instances for some features, we need to manually update all references here and in the SDK, potentially introducing bugs.

One solution is to maintain a central config or constants file with name mappings like SM_INSTANCE_NAME_P4D ='ml.p4d.24xlarge'; and then another central mapping for `SUPPORTED_INSTANCE_TYPES_SMDDP`, `SUPPORTED_INSTANCE_TYPES_EFA`, etc.

Contributor guide

Open the contributing guide

Research direction

Start by reading src/sagemaker_training/mpi.py, test/unit/test_smdataparallel.py, and src/sagemaker_training/pytorch_xla.py at the referenced locations. Compare the instance-type literals in those files with the related Python SDK references mentioned in the issue. Done means the relevant names and supported-instance mappings have a single maintained source, with tests and validation updated consistently.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
machine-learning
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.