Fixed Typehint for RunSettings.colocated_db_settings #462
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #462 +/- ##
===========================================
- Coverage 90.42% 81.42% -9.00%
===========================================
Files 60 60
Lines 3738 3834 +96
===========================================
- Hits 3380 3122 -258
- Misses 358 712 +354
|
smartsim/settings/lsfSettings.py
Outdated
| db_cpus = int(self.colocated_db_settings.get("db_cpus", 0)) | ||
| db_cpus = self.colocated_db_settings.get("db_cpus", 0) | ||
| assert isinstance(db_cpus, int) |
There was a problem hiding this comment.
Minor refactor request here to make sure this is not a breaking API change
db_cpus = int(t.cast(int, self.colocated_db_settings.get("db_cpus", 0)))
# and then you should be free to remove the `assert stmt` as well!There was a problem hiding this comment.
If Im already casting this to an int, why again?
There was a problem hiding this comment.
t.cast is purely for the type checker while the int will actually do the cast at runtime. It's HIGHLY unlikely, but there is the possibility that the "db_cpus" key maps to a float (or a str, or some other type) that can be cast to an int.
so for instance
self.colocated_db_settings = {"db_cpus": 3.0, ...}
db_cpus = int(self.colocated_db_settings.get("db_cpus", 0))
print(db_cpus)will run just fine and print 3
while
self.colocated_db_settings = {"db_cpus": 3.0, ...}
db_cpus = self.colocated_db_settings.get("db_cpus", 0)
assert isinstance(db_cpus, int)
print(db_cpus)will raise an AssertionError.
Just to make sure that this is preserving the original behavior and prevent an API break I would rather just subtly lie to the type checker and let it think that self.colocated_db_settings.get("db_cpus", 0) is some type that can be cast to an int. But do LMK if you think I am being too paranoid!
There was a problem hiding this comment.
I see I see, some witchery I would say
smartsim/entity/model.py
Outdated
| x = t.cast( | ||
| t.Optional[t.Iterable[t.Union[int, t.Iterable[int]]]], | ||
| common_options.get("custom_pinning"), | ||
| ) | ||
|
|
||
| colo_db_config = {} | ||
| e = t.cast(int, common_options.get("cpus")) | ||
| common_options["custom_pinning"] = self._create_pinning_string(x, e) |
There was a problem hiding this comment.
Can we change the names of these of these intermediary variables to something a bit more descriptive (maybe custom_pinning_ and cpus_)?
Or maybe just remove them entirely?
common_options["custom_pinning"] = self._create_pinning_string(
t.cast(
t.Optional[t.Iterable[t.Union[int, t.Iterable[int]]]],
common_options["custom_pinning"], common_options["cpus"]
),
t.cast(int, common_options.get("cpus"))
)There was a problem hiding this comment.
I would almost suggest keeping the vars for readability, but Im not as experienced so let me know!
There was a problem hiding this comment.
Totally willing to keep the intermediate variables! I think I agree that they improve readability!
This PR adds Python type hinting to RunSettings.colocated_db_settings. [ reviewed by @MattToast ] [ committed by @amandarichardsonn ]
This PR adds Python type hinting to RunSettings.colocated_db_settings