Skip to content

Commit e35bb2f

Browse files
author
Harsh Thakkar
committed
fix: skip None hyperparameters in to_dict instead of converting to string 'None'
1 parent 4c184d4 commit e35bb2f

2 files changed

Lines changed: 57 additions & 1 deletion

File tree

sagemaker-train/src/sagemaker/train/common.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ def __init__(self, options_dict: Dict[str, Any]):
3333

3434
def to_dict(self) -> Dict[str, Any]:
3535
"""Convert back to dictionary for hyperparameters with string values."""
36-
return {k: str(getattr(self, k)) for k in self._specs.keys()}
36+
return {k: str(v) for k in self._specs.keys() if (v := getattr(self, k)) is not None}
3737

3838
def __setattr__(self, name: str, value: Any):
3939
if name.startswith('_'):
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
from sagemaker.train.common import FineTuningOptions
2+
3+
4+
class TestFineTuningOptionsToDict:
5+
"""Tests for FineTuningOptions.to_dict() None value handling."""
6+
7+
def test_to_dict_skips_none_values(self):
8+
"""None-valued hyperparameters should be omitted from to_dict output."""
9+
options = FineTuningOptions({
10+
"learning_rate": {"default": 0.0002, "type": "float"},
11+
"resume_from_path": {"default": None, "type": "string"},
12+
"global_batch_size": {"default": 64, "type": "integer"},
13+
})
14+
result = options.to_dict()
15+
assert "resume_from_path" not in result
16+
assert result == {"learning_rate": "0.0002", "global_batch_size": "64"}
17+
18+
def test_to_dict_includes_non_none_values(self):
19+
"""Non-None values should be included as strings."""
20+
options = FineTuningOptions({
21+
"learning_rate": {"default": 0.001, "type": "float"},
22+
"max_epochs": {"default": 3, "type": "integer"},
23+
"model_name": {"default": "my-model", "type": "string"},
24+
})
25+
result = options.to_dict()
26+
assert result == {
27+
"learning_rate": "0.001",
28+
"max_epochs": "3",
29+
"model_name": "my-model",
30+
}
31+
32+
def test_to_dict_empty_string_is_included(self):
33+
"""Empty string is a valid value and should not be skipped."""
34+
options = FineTuningOptions({
35+
"mlflow_run_id": {"default": "", "type": "string"},
36+
})
37+
result = options.to_dict()
38+
assert result == {"mlflow_run_id": ""}
39+
40+
def test_to_dict_after_user_sets_none_to_value(self):
41+
"""If user overrides a None default with a real value, it should appear."""
42+
options = FineTuningOptions({
43+
"resume_from_path": {"default": None, "type": "string"},
44+
})
45+
options.resume_from_path = "/path/to/checkpoint"
46+
result = options.to_dict()
47+
assert result == {"resume_from_path": "/path/to/checkpoint"}
48+
49+
def test_to_dict_all_none_returns_empty(self):
50+
"""If all values are None, to_dict should return empty dict."""
51+
options = FineTuningOptions({
52+
"param_a": {"default": None, "type": "string"},
53+
"param_b": {"default": None, "type": "string"},
54+
})
55+
result = options.to_dict()
56+
assert result == {}

0 commit comments

Comments
 (0)