MNT Add light type annotations to skops.io by BenjaminBossan · Pull Request #219 · skops-dev/skops

BenjaminBossan · 2022-11-28T14:05:59Z

This is in line with the rest of the code base.

On top of adding those type annotations, these changes were made:

Moved all code from _dispatch.py to _audit.py. The name _dispatch.py didn't make much sense after the refactor, _audit.py only had 2 functions, and there were also problems with circular imports.
Small refactor in FunctionNode: get_unsafe_set used to call is_safe, which is the reverse of how all other Nodes do it. Changed it to be consistent.
Fixed a bug in ReduceNode: The raised exception would access self.constructor, which does not exist.
Add a few comments to Node.__init__ about what subclasses should implement.
Add _construct method to Node, which raises NotImplementedError
Add TODO comment to CachedNode as it doesn't deal with unknown id yet.
gettype now raises an error when the type could not be determined, it cannot return None anymore.
get_untrusted_types now deals with neither data nor file being passed.

Unfortunately, due 1., this PR looks much bigger than it is, since most of the changes are just moving from one module to the other. I would recommend diffing the new _audit.py against the old _dispatch.py to see the actual changes.

This is in line with the rest of the code base. On top of adding those type annotations, these changes were made: 1. Moved all code from _dispatch.py to _audit.py. _dispatch.py didn't make much sense after the refactor and there were also problems with circular imports. 2. Small refactor in FunctionNode: get_unsafe_set used to call is_safe, which is the inverse of how all other Nodes do it. Changed it to be consistent. 3. Fixed a bug in ReduceNode: Exception would access self.constructor, which does not exist. 4. Add a few comments to Node.__init__ about what subclasses should implement. 5. Add _construct method to Node, raise NotImplementedError 6. Add TODO comment to CachedNode as it doesn't deal with unknown id yet. 7. gettype now raises error when type could not be determined, cannot return None anymore 8. get_untrusted_types now deals with neither data nor file being passed

BenjaminBossan · 2022-11-28T14:53:54Z

@skops-dev/maintainers ready for review

E-Aho

Overall LGTM, minor todo and quesiton I would like to ask before approving

E-Aho · 2022-11-28T19:36:46Z

        }

-    def _construct(self):
+    def _construct(self) -> Any:


This comment applies to basically all of the _construct functions:

This always return a slice, so shouldn't we change the type to -> slice:?

I actually considered that, but decided against it.

The reason is that those types would not help us in our code. That's because everywhere we call _construct, we cannot statically know which of the dozens of types it would return. And this is because we work with generic states. If we had type-specific states, like DictState or NdArrayState, we may be able to add overloads that say what type is returned by _construct based on the state, but we don't have that (and it would be difficult to implement, maybe even impossible with the current typing situation).

So if we added return type annotations here, the only thing that mypy could infer is that _construct returns a Union[str, int, float, dict, list, NDArray, ...], which is not more useful than Any in practice.

In that case, why do we then need to have the type annotation in the first place? can't we just remove it?

adrinjalali

Thanks @BenjaminBossan

adrinjalali · 2022-11-29T10:06:42Z

+
+
+@contextmanager
+def temp_setattr(obj: Any, **kwargs: Any) -> Generator[None, None, None]:


What does Generator[None, None, None] mean? typehints should helps us understand the code better, not to make them more confusing :D

Also, if everything is Any here, then why do we have typehints?

This is an example where I wouldn't add any typehints here.

The meaning for Generator is Generator[YieldType, SendType, ReturnType], which happens to be None, None, None here. I agree that types don't add much here, though **kwargs: Any is a shorthand for dict[str, Any]. The difference for me between a function annotated with Any vs no annotation is that with the latter, I don't know if it takes any or if it was just not annotated yet, so I guess not completely useless.

I would thus keep the annotations here, but if you insist, I can remove them.

I think typehints should be useful, if they're not, they take extra space and make things less readable.

I agree with Adrin

imo, type hints should really be used like docstrings, if having them just takes up space and doesn't make things clearer or help an IDE, they probably don't need to be there.

adrinjalali · 2022-11-29T10:17:32Z

+        self,
+        state: dict[str, Any],
+        load_context: LoadContext,
+        constructor: Type[Any],


can also me a function.

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

BenjaminBossan · 2022-11-29T13:31:51Z

@E-Aho @adrinjalali I made some suggested changes and replied to the other comments, please take another look.

adrinjalali

I'm happy with most annotations here, but I don't think we should annotate everything for the sake of having everything annotated. If it doesn't add much, we shouldn't add it then.

adrinjalali · 2022-11-29T14:36:13Z

+
+
+@contextmanager
+def temp_setattr(obj: Any, **kwargs: Any) -> Generator[None, None, None]:


I think typehints should be useful, if they're not, they take extra space and make things less readable.

E-Aho

Overall looks really good, thanks!
I do think we can probably remove the -> Any:s and the Nones inside the Generator type hint, but I'd also be ok with including them if you think they're more helpful to have than not to have here.

E-Aho · 2022-11-29T19:05:36Z

+
+
+@contextmanager
+def temp_setattr(obj: Any, **kwargs: Any) -> Generator[None, None, None]:


I agree with Adrin

imo, type hints should really be used like docstrings, if having them just takes up space and doesn't make things clearer or help an IDE, they probably don't need to be there.

BenjaminBossan · 2022-11-30T10:43:04Z

Replying here since the comments are a bit distributed right now @E-Aho @adrinjalali

I do think we can probably remove the -> Any:s and the Nones inside the Generator type hint

Okay, I'll remove the -> Anys from _construct, as those indeed add no further information.

I would not remove the Nones from Generator, since they do indicate information, e.g. if the context manager returns something. So if someone wrote code like

with temp_setattr(...) as foo:
    foo.bar()

mypy knows that it doesn't work because we specified that the yield type is None.

Speaking more generally, Generator, like List, is a generic type and should be further specified if possible (though I admit that it looks strange at first).

Regarding the Anys in cases like this:

def temp_setattr(obj: Any, **kwargs: Any)

as I argued, it provides information, namely that this works on any object and that the dict values can also be any objects. Without types, I wouldn't know if I can pass anything or if the function was just not typed but does have restrictions. Therefore, I'd argue that this doesn't apply:

I think typehints should be useful, if they're not, they take extra space and make things less readable.

So I'd leave the types here. Agree?

It is not adding value.

adrinjalali · 2022-11-30T12:43:00Z

Without types, I wouldn't know if I can pass anything or if the function was just not typed but does have restrictions.

I think for that we need documentation (docstrings), not typehints.

BenjaminBossan · 2022-11-30T12:50:47Z

I think for that we need documentation (docstrings), not typehints.

We can have both, no :) And generally, type hints are better supported in editors than docstrings.

E-Aho · 2022-11-30T18:17:51Z

I definitely fully agree with Benjamin on type hints being handy inside IDEs.

I am happy to add the rest of the type hints.

I think though it could maybe be useful to also document what that generator return actually will behave like, as it does look a little odd at first glance to see something returning Generator[None, None, None] if you aren't expressly familiar with that typing syntax

E-Aho

I'm happy to approve this, optionally add a small comment about the format of the Generator[None,...] output type as it's a bit confusing at first glance if you aren't familiar with that syntax, but LGTM in any case

BenjaminBossan · 2022-12-01T09:55:35Z

I added a comment to explain the types.

adrinjalali

Thanks @BenjaminBossan , nice work.

One point: I kinda said from the beginning when we started doing typehints, that I'm fine as long as we don't end up insisting on having hints everywhere. We're somewhat going that way now and that doesn't make me happy. It should be fine for methods to have no typehints.

BenjaminBossan · 2022-12-01T10:34:38Z

Yes, I'm not married to type annotations and there are sure situations where they can be quite annoying or where mypy is just not powerful enough to understand what's going on. An example we discussed was a type for sklearn estimators, which would be too much of a hassle to implement.

For this particular PR, I should note, however, that types really did help. They uncovered issue 2, 3, 6, 7, and 8 in my first comment. Was it necessary to add all the types to find those? No. But we don't know beforehand where types can help, so when in doubt, I would err towards adding them.

BenjaminBossan added 2 commits November 28, 2022 14:57

Python 3.7: Import Literal from fixes

1d610f9

E-Aho reviewed Nov 28, 2022

View reviewed changes

adrinjalali reviewed Nov 29, 2022

View reviewed changes

BenjaminBossan and others added 4 commits November 29, 2022 12:06

Update skops/io/_audit.py

975283d

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Type annotation: constructor can be a function too

b984727

Add returns part of get_tree docstring

b790323

Fix an error in persistence documentation

8da4bdd

adrinjalali reviewed Nov 29, 2022

View reviewed changes

E-Aho reviewed Nov 29, 2022

View reviewed changes

Remove Any return type annotation from _construct

662b593

It is not adding value.

E-Aho approved these changes Dec 1, 2022

View reviewed changes

Add comment explaining Geneator type annotation

9358f36

adrinjalali approved these changes Dec 1, 2022

View reviewed changes

adrinjalali changed the title ~~Add light type annotations to skops.io~~ MNT Add light type annotations to skops.io Dec 1, 2022

adrinjalali merged commit 69763fe into skops-dev:main Dec 1, 2022

BenjaminBossan deleted the audit-type-annotations branch December 1, 2022 10:34



		@contextmanager
		def temp_setattr(obj: Any, **kwargs: Any) -> Generator[None, None, None]:

Conversation

BenjaminBossan commented Nov 28, 2022

Uh oh!

BenjaminBossan commented Nov 28, 2022

Uh oh!

E-Aho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BenjaminBossan commented Nov 29, 2022

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

E-Aho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan commented Nov 30, 2022

Uh oh!

adrinjalali commented Nov 30, 2022

Uh oh!

BenjaminBossan commented Nov 30, 2022

Uh oh!

E-Aho commented Nov 30, 2022

Uh oh!

E-Aho left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan commented Dec 1, 2022

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan commented Dec 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants