-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Typed automodel and type checking during tests. #957
Typed automodel and type checking during tests. #957
Conversation
Do not merge, I need to add a test to ensure the types are correct. |
We need to merge #960 before for this PR to work. |
About how this work: Typeguard has a pytest plugin. This plugin will hook into all autokeras imports and run the Meaning that during the tests, every time a function with type hints is called, the arguments are checked (as opposed to the default behavior in python, where type hints do nothing). If the type hints are wrong, here is the result: https://github.com/keras-team/autokeras/runs/440879952 |
Ready for review |
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 for the PR!
autokeras/auto_model.py
Outdated
tuner='greedy', | ||
overwrite=False, | ||
seed=None): | ||
inputs: Union[Node, List[Node]], |
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.
Please change Node to Input (ak.nodes.Input)
autokeras/auto_model.py
Outdated
overwrite=False, | ||
seed=None): | ||
inputs: Union[Node, List[Node]], | ||
outputs: Union[Block, Node, list], |
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.
Change Block to Head (ak.engine.head.Head)
setup.py
Outdated
@@ -32,7 +32,8 @@ | |||
'isort', | |||
'pytest-xdist', | |||
'pytest-cov', | |||
'coverage' | |||
'coverage', | |||
'typeguard==2.7.1' |
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.
why are we pinning the version of this one?
Is it stable?
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.
It's stable, I'm just a bit too careful sometimes haha
autokeras/auto_model.py
Outdated
overwrite=False, | ||
seed=None): | ||
inputs: Union[Input, List[Input]], | ||
outputs: Union[head_module.Head, list], |
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.
It should be Union[Head, Node, list]
. I think you accidentally removed Node
.
Another concern is how will it show on the docs by autogen.py?
Will it be head_module.Head
directly? That would be confusing for the reader.
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, I'll fix it.
Good question, I'll dig into it. I'm planning to change the code generation too to display the types in the description, not the signature as it's too cumbersome and hurts readability.
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.
Here is what it looks like in the docs:
autokeras.AutoModel(
inputs: Union[autokeras.nodes.Input, List[autokeras.nodes.Input]],
outputs: Union[autokeras.engine.head.Head, list],
name: str = "auto_model",
max_trials: int = 100,
directory: Union[str, pathlib.Path, None] = None,
objective: str = "val_loss",
tuner: Union[str, Type[autokeras.engine.tuner.AutoTuner]] = "greedy",
overwrite: bool = False,
seed: Optional[int] = None,
)
I can see two things to fix:
- Fist, use the aliases in the type hints instead of the complete class path.
- Second, display those info in the arguments descriptions only.
I'll open two issue in keras-autodoc. I'll fix the aliases issue first, and I believe we can merge this PR once it's done to avoid blocking the typing of autokeras.
The second issue is more or less making the docs beautiful, which is important, but not critical.
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.
It looks like that now:
autokeras.AutoModel(
inputs: Union[autokeras.Input, List[autokeras.Input]],
outputs: Union[autokeras.Head, autokeras.Node, list],
name: str = "auto_model",
max_trials: int = 100,
directory: Union[str, pathlib.Path, None] = None,
objective: str = "val_loss",
tuner: Union[str, Type[autokeras.engine.tuner.AutoTuner]] = "greedy",
overwrite: bool = False,
seed: Optional[int] = None,
)
AutoTuner
is not listed in the docs, so there is no aliases, this is why keras-autodoc can't use an alias for it.
Do we plan to make AutoTuner
public in the future?
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.
Yes, we should make it public for anyone would like to override to implement their own tuner. I will merge this PR now.
Would you also help us make typedapi part of the setup.py tests dependencies? That would make the community easier to setup the environment. Thank you!
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.
Sure thing, I'll upload it on pypi when I have the time :)
Linked to #949