aws / aws/sagemaker-training-toolkit
Remove magic strings for attributes like instance type
- 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
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