Add typehints to much of the core code. #379
Conversation
…con-ml into spardy/typehints
| @@ -9,19 +19,22 @@ class Base: | |||
| The config, which injects the repository to use. | |||
| """ | |||
|
|
|||
| def __init__(self, domain, config=None): | |||
| def __init__(self, domain: DOMAIN_TYPES, config: Optional[Config] = None): | |||
There was a problem hiding this comment.
I have to look into why the config is optional - that doesn't seem right
There was a problem hiding this comment.
Yeah, the default value here is None, do we need to make it required?
There was a problem hiding this comment.
not in this PR, maybe in the future. I think its a remnant of the fact that Rubicon used to extend the Base and didn't take in a config since it generated it. I'll verify that's the case and if so make an issue to fix it
| @@ -29,18 +35,22 @@ class Config: | |||
| """ | |||
|
|
|||
| PERSISTENCE_TYPES = ["filesystem", "memory"] | |||
| REPOSITORIES = { | |||
| REPOSITORIES: Dict[str, Type[BaseRepository]] = { | |||
There was a problem hiding this comment.
why do we need the Type here?
There was a problem hiding this comment.
Type here means that the dictionary contains uninstantiated classes.
rubicon_ml/client/mixin.py
Outdated
| @@ -47,14 +60,14 @@ def _validate_data(self, data_bytes, data_file, data_object, data_path, name): | |||
| @failsafe | |||
| def log_artifact( | |||
| self, | |||
| data_bytes=None, | |||
| data_bytes: Optional[bytes] = None, | |||
| data_file=None, | |||
rubicon_ml/client/mixin.py
Outdated
| @failsafe | ||
| def log_dataframe(self, df, description=None, name=None, tags=[]): | ||
| def log_dataframe( | ||
| self, df: Union[pd.DataFrame, dd.DataFrame], description=None, name=None, tags=[] |
| """Get the parameter's name.""" | ||
| return self._domain.name | ||
|
|
||
| @property | ||
| def value(self): | ||
| def value(self) -> Optional[Union[object, float]]: |
There was a problem hiding this comment.
isn't Union[object, float] == object?
rubicon_ml/client/parameter.py
Outdated
| """Get the parameter's value.""" | ||
| return self._domain.value | ||
| return getattr(self._domain, "value", None) |
There was a problem hiding this comment.
why is this necessary? a parameter domain should never not have a value key
There was a problem hiding this comment.
Good catch. This was because I didn't hint the _domain value properly.
rubicon_ml/client/rubicon.py
Outdated
| root_dir=None, | ||
| auto_git_enabled=False, |
Partially addresses #368
Adds partial type hints throughout the code.
Leaves almost all logic untouched. One exception are areas where the default argument was a list. This has been changed to
Nonewith the list added in the function body. This is to avoid the mutable default argument issue.What
How to Test
mypy- will still show around 100 issues.