Added typing information for all public APIs #1158
Conversation
The docstrings were also slightly more standardized. Only public facing APIs had the type information added to them. There are a few minor bug fixes in the default values for some functions (namely using [] as a default value (replaced with None)) as well as removing default values where it does not make sense (S3.info() for example). There should be no other functional change.
|
I have a typo -- will fix. |
|
I am still working on this to make it fully compatible with the dev command being developed. It's mostly formatting the strings and what not but want to do it only once :). |
metaflow/datatools/s3.py
Outdated
| tmproot: str = ".", | ||
| bucket: Optional[str] = None, | ||
| prefix: Optional[str] = None, | ||
| run: Optional[Union[FlowSpec, TRun]] = None, |
There was a problem hiding this comment.
Been staring at this for a moment. Why the use of a TypeVar? I know these can be used for generics to enhance autocompletion when using subclasses.
Example:
from typing import Generic, TypeVar
from dataclasses import dataclass
from .some_file import Weapon
TWeapon = TypeVar("TWeapon", bound=Weapon)
@dataclass
class Enemy(Generic[TWeapon]):
holding: TWeapon
enemy = Enemy(holding=...some weapon subclass instance)
enemy.holding # gives autocompletion for the specific subclass of Weapon in holdingIs that somehow what's going on with TRun? Does Union[type, type] simply require TypeVar objects rather than the string representation, i.e. "Run"?
There was a problem hiding this comment.
Let me try with "Run". I have to check it works on 3.5 too. IIRC that may be why I went this route but may have been delusional :) .
|
@romain-intel thanks for taking this on! Fingers crossed this gets accepted :) |
|
Thanks for the review. I need to rebase this and clean it up some more but yes, I am also hoping we can get this in. @tuulos , what do you think? |
|
sorry for the delay @phitoduck -- I am back to working on this to address your comments, update and hopefully merge soon :). |
|
Testing[313] @ 5e8e6dc |
|
Testing[313] @ 5e8e6dc had 15 FAILUREs. |
|
Testing[313] @ d3f515a |
|
Testing[313] @ 302d7b4 |
| Iterator over `Run` objects in this flow. | ||
| """ | ||
| return self._filtered_children(*tags) | ||
|
|
||
|
|
||
| class Metaflow(object): |
There was a problem hiding this comment.
Any reasons to move this object to the end? Are there any diffs here that I should be looking into?
There was a problem hiding this comment.
it needed Flow iirc.
There was a problem hiding this comment.
There should be no change.
| @@ -50,19 +50,14 @@ def unquote_bytes(x): | |||
|
|
|||
| from shlex import quote as _quote | |||
|
|
|||
| if sys.version_info >= (3, 7): | |||
There was a problem hiding this comment.
what's the context for this change?
There was a problem hiding this comment.
so I could use NamedTuple and have the proper types.
| `@resources` is also present, the maximum value from all decorators is | ||
| used. | ||
| image : str | ||
| image : str, optional, default: None |
There was a problem hiding this comment.
this default isn't wholly accurate
There was a problem hiding this comment.
I put it as optional and described tthe behavior in the doc string.
There was a problem hiding this comment.
the behavior is symmetric to batch - so the comment should be quite similar here
There was a problem hiding this comment.
stupid me -- I had modified it but forgot to save the file.
There was a problem hiding this comment.
stupid me -- I had modified it but forgot to save the file.
| Kubernetes service account to use when launching pod in Kubernetes. | ||
| namespace : str, default: METAFLOW_KUBERNETES_NAMESPACE | ||
| Kubernetes namespace to use when launching pod in Kubernetes. | ||
| secrets : List[str], optional, default: None |
There was a problem hiding this comment.
the default would be METAFLOW_KUBERNETES_SECRETS
There was a problem hiding this comment.
I left to optional beceause the doc string and code actually add it to KUBERNETES_SECRETS
| Kubernetes service account to use when launching pod in Kubernetes. | ||
| namespace : str, default: METAFLOW_KUBERNETES_NAMESPACE | ||
| Kubernetes namespace to use when launching pod in Kubernetes. | ||
| secrets : List[str], optional, default: None | ||
| Kubernetes secrets to use when launching pod in Kubernetes. These | ||
| secrets are in addition to the ones defined in `METAFLOW_KUBERNETES_SECRETS` | ||
| in Metaflow configuration. |
There was a problem hiding this comment.
default for tolerations would be METAFLOW_KUBERNETES_TOLERATIONS
|
Testing[313] @ dac5fbe |
|
Testing[313] @ 302d7b4 had 1 FAILURE. |
|
Testing[313] @ dac5fbe PASSED |
The docstrings were also slightly more standardized.
Only public facing APIs had the type information added to them.
There are a few minor bug fixes in the default values for some functions (namely using [] as a default value (replaced with None)) as well as removing default values where it does not make sense (S3.info() for example). There should be no other functional change.