Add Type Check Back to Refactor CI #630
Conversation
MattToast
added
the
area: CI/CD
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## smartsim-refactor #630 +/- ##
====================================================
Coverage ? 32.89%
====================================================
Files ? 106
Lines ? 6447
Branches ? 0
====================================================
Hits ? 2121
Misses ? 4326
Partials ? 0
|
| @@ -8,14 +10,11 @@ | |||
| from .basejob import BaseJob | |||
|
|
|||
|
|
|||
| class BaseJobGroup(Launchable, MutableSequence): | |||
There was a problem hiding this comment.
That was my bad, nice catch!
There was a problem hiding this comment.
How did the tests still pass even though I did not inherit the ABC class?
There was a problem hiding this comment.
Without going too far into the weeds of how ABC and abstractmethod work, the @abstractmethod decorator just sets a __isabstractmethod__ = True attr to the method.
class C:
@abstractmethod
def some_method(self): print("Totally works!")
C().some_method() # prints: "Totally works!"
C().some_method.__isabstractmethod__ # returns: `True`The ABC class is what actually enforces that no methods have a __isabstractmethod__ set to true when the class is created:
class C(ABC):
@abstractmethod
def some_method(self): ...
C() # Now cannot be instanced, raises a `TypeError`So basically, it got past the tests precisely because it did not inherit from ABC!
| raise TypeError("Can only assign a `BaseJob`") | ||
| self.jobs[idx] = deepcopy(value) | ||
| else: | ||
| if not isinstance(value, t.Iterable): |
There was a problem hiding this comment.
Do you need to check here that its an iterable of BaseJobs?
There was a problem hiding this comment.
If we want to keep it type safe at runtime, we unfortunately do :(.
From the overloads, we can tell that when idx is a slice that value should be a Iterable, but unfortunately that assumed constraint is not actually guaranteed at runtime, and thus the type checker will complain for the definition of this method if we do not explicitly handle it.
|
|
||
| def __setitem__(self, idx: int, value: BaseJob) -> None: | ||
| @t.overload | ||
| def __setitem__(self, idx: int, value: BaseJob) -> None: ... |
There was a problem hiding this comment.
why is setitem repeated twice and what is t.overload?
There was a problem hiding this comment.
__setitem__ is actually defined here 3 times! The first two definitions give the different signatures to the type checker to provide more information about how a method should be used. Only the last one is the one that matters for runtime though.
Python does not have proper method overloading (each method has one, and exactly one, definition), but the @t.overload decorator informs the type checker that depending on how the method is called there are really many (in this case two) discrete signatures. This allows for python to "spoof" true method overloading at type check time.
Now if you index into the job group with an int mypy knows that you should be trying to assign a BaseJob, but if you index into the job group with a slice, mypy knows that it should expect an Iterable[BaseJob]
| @@ -26,9 +31,19 @@ def jobs(self) -> t.List[BaseJob]: | |||
| """ | |||
| return self._jobs | |||
|
|
|||
| def __str__(self): # pragma: no-cover | |||
| @t.overload | |||
| def __getitem__(self, idx: int) -> BaseJob: ... | |||
There was a problem hiding this comment.
repeated dunder methods again, hmmmm
There was a problem hiding this comment.
Similar to the __setitem__ conversation, only the last definition matters at runtime, but these other methods provide more information to the type checker.
Now if a user calls this method with different indexes, mypy is able to infer what type you should get back
reveal_type(job_group[0]) # reveals: `int`
reveal_type(job_group[1:3]) # reveals: `JobGroup`but do note that it is still up to the author to make sure that the actual definition conforms to the method overloads. In this case, it's basically a fancy way to the the type checker "trust me: when you get this input you can expect this output".
| class DragonRunSettings: | ||
| pass | ||
| class SettingsBase: | ||
| def __init__(self, *_: t.Any, **__: t.Any) -> None: ... |
There was a problem hiding this comment.
why did u define dunders for base?
There was a problem hiding this comment.
Just to get past the type check without ignoring all of _core. There are still a few places where settings are instanced using one of these stubclasses. If we do not define a constructor that takes any number of args and any number of kwargs, the type checker will complain anytime a settings is instanced that does not conform to the signature
There was a problem hiding this comment.
Similarly, the __getattr__ is there so that the type checker does not complain when try to dot-access an attr that does not directly exist on these stub classes.
| @@ -10,8 +10,13 @@ | |||
| app_3 = Application("app_3", "python", run_settings=LaunchSettings("slurm")) | |||
|
|
|||
|
|
|||
| class MockJob(BaseJob): | |||
There was a problem hiding this comment.
why did you switch out BaseJob for MockJob?
There was a problem hiding this comment.
We need to provide a concrete implementation of the BaseJob abstract class. Otherwise we cannot instance the class we are testing!
Addresses type errors found in the SmartSim core refactor branch. Re-adds a static type check back into the CI to allow for gradual typing as more parts of the API become more concrete [ committed by @MattToast ] [ reviewed by @amandarichardsonn ]
Addresses type errors found in the SmartSim core refactor branch. Re-adds a static type check back into the CI to allow for gradual typing as more parts of the API become more concrete [ committed by @MattToast ] [ reviewed by @amandarichardsonn ]
Addresses type errors found in the SmartSim core refactor branch. Re-adds a static type check back into the CI to allow for gradual typing as more parts of the API become more concrete