Sbachmei/mic 5347/mypy resources #490
Conversation
stevebachmeier
requested review from
albrja,
collijk,
hussain-jafari,
patricktnast and
rmudambi
as code owners
src/vivarium/framework/resource.py
Outdated
| producer: Callable, | ||
| dependencies: List[str], | ||
| resource_names: list[str], | ||
| producer: Callable[..., Any], |
There was a problem hiding this comment.
I'm not 100% clear on this, but I think a producer really can take anything as an arg and return anything? Would like to talk about it w/ someone who is more familiar w this resource stuff.
| @@ -144,24 +149,24 @@ def sorted_nodes(self): | |||
| """ | |||
| if self._sorted_nodes is None: | |||
| try: | |||
| self._sorted_nodes = list(nx.algorithms.topological_sort(self.graph)) | |||
| self._sorted_nodes = list(nx.algorithms.topological_sort(self.graph)) # type: ignore[func-returns-value] | |||
There was a problem hiding this comment.
This was giving an error that nx.algorithms.topological_sort alwasy returns None which simply isn't true. I didn't dig too deep but assume that the stub package I downloaded (which is not by the same folks who maintain networkx) simply typed it wrong.
There was a problem hiding this comment.
Is it worth contributing a PR to the stubs package to fix this?
There was a problem hiding this comment.
Interesting, I'll take a look. I've never contributed to a 3rd party package before so that would be fun
| @@ -44,6 +44,7 @@ | |||
| "dill", | |||
| # Type stubs | |||
| "pandas-stubs", | |||
| "networkx-stubs", | |||
There was a problem hiding this comment.
a stub package for networkx
| @@ -250,7 +255,7 @@ def _to_graph(self) -> nx.DiGraph: | |||
| if dependency not in self._resource_group_map: | |||
| # Warn here because this sometimes happens naturally | |||
| # if observer components are missing from a simulation. | |||
| self.logger.warning( | |||
| self.logger.warning( # type: ignore[no-untyped-call] | |||
There was a problem hiding this comment.
untyped from loguru. I tried using the loguru-mypy package but it didn't get rid of this particular error wo I just ignore it
| @@ -44,6 +44,7 @@ | |||
| "dill", | |||
| # Type stubs | |||
| "pandas-stubs", | |||
| "networkx-stubs", | |||
There was a problem hiding this comment.
a stub package for networkx
src/vivarium/framework/resource.py
Outdated
| @@ -335,7 +340,7 @@ def add_resources( | |||
| """ | |||
| self._manager.add_resources(resource_type, resource_names, producer, dependencies) | |||
|
|
|||
| def __iter__(self): | |||
| def __iter__(self) -> Iterator[Callable[..., Any]]: | |||
There was a problem hiding this comment.
I think this is correct but I'd like to talk about this w/ someone familiar with python's implementation of iterables
| # This will be a dict with string keys representing the the resource | ||
| # and the resource group they belong to. This is a one to many mapping | ||
| # as some resource groups contain many resources. | ||
| self._resource_group_map = {} | ||
| self._resource_group_map: dict[str, ResourceGroup] = {} |
There was a problem hiding this comment.
Can you convert these inline comments to docstrings?
There was a problem hiding this comment.
I thought about doing that, but they won't show up b/c they're private anyway. Are you suggesting just so it's easier to know which comment goes w/ wihich attr?
| @@ -144,24 +149,24 @@ def sorted_nodes(self): | |||
| """ | |||
| if self._sorted_nodes is None: | |||
| try: | |||
| self._sorted_nodes = list(nx.algorithms.topological_sort(self.graph)) | |||
| self._sorted_nodes = list(nx.algorithms.topological_sort(self.graph)) # type: ignore[func-returns-value] | |||
There was a problem hiding this comment.
Is it worth contributing a PR to the stubs package to fix this?
stevebachmeier
force-pushed
the
sbachmei/mic-5347/mypy-resources
branch
from
e624482 to
3818782
Compare
fix mypy errors: framework/resource.py
Description
Changes and notes
Testing