-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add typing check #50
Add typing check #50
Conversation
fondant/component_spec.py
Outdated
@@ -239,7 +244,7 @@ def from_fondant_component_spec( | |||
@staticmethod | |||
def _dump_args(args: t.List[Argument]) -> t.List[t.Union[str, t.Dict[str, str]]]: | |||
"""Dump Fondant specification arguments to kfp command arguments.""" | |||
dumped_args = [] | |||
dumped_args: t.List[t.Union[str, t.Dict[str, str]]] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh I find these typing hints pretty hard to read and I'm not sure they're helpful for us. They seem useful for computers but not for humans
t.List[t.Union[str, t.Dict[str, str]]]
=> I mean just look at this
Just my personal opinion :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I twas getting errors from mypy
as follows
Incompatible return value type (got "List[str]", expected "List[Union[str, Dict[str, str]]]") which went away when I explicitly defined the type.
However, the args of kubeflow does required to have a list that can contains either a string or a dictionary so in that sense I think the type hint in the function's return is valid.
I'll just add a mypy exception for that one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The typehints are meant to be useful for computers though, that's why we are adding mypy
:)
If we have complex types that are reused across Fondant, we can create aliases for them. Eg:
https://github.com/encode/starlette/blob/master/starlette/types.py
I would rather have a correct complex type than ignore the issue. Defining the types helps catch bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright I think that's a better approach indeed :) thanks for the suggestion. Added
fondant/manifest.py
Outdated
|
||
spec_data = pkgutil.get_data("fondant", "schemas/manifest.json") | ||
|
||
if spec_data is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but the following is a bit easier to follow:
spec_data = pkgutil.get_data("fondant", "schemas/manifest.json")
if spec_data is None:
raise FileNotFoundError("component_spec.json not found in fondant schema")
spec_str = spec_data.decode("utf-8")
spec_schema = json.loads(spec_str)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @PhilippeMoussalli!
I would revert ignoring the args type, but otherwise LGTM.
This PR adds type checking into the CICD pipelines of Fondant. We're leveraging the latest version of mypy. #46
This PR adds type checking into the CICD pipelines of Fondant. We're leveraging the latest version of mypy.
#46