Add typehints for `fsspec.caching` and `fsspec.utils` #1284

alanhdu · 2023-06-01T20:06:45Z

This add typehints to fsspec.caching and fsspec.utils (as best as I can understand them), and then turns on check_untyped_defs for them as well (so that every function in those files will be type-checked, note just those with type annotations).

alanhdu · 2023-06-01T20:12:00Z

fsspec/utils.py

+    if length is not None:
+        b = f.read(length)
+    else:
+        b = f.read()


I assume we needed to do this (otherwise if delimiter is None and length is None I'm pretty sure this crashes), but happy to revert this.

Let's not change the logic in this PR. I am not immediately sure about what you suggest.

Sure -- can revert and add a type: ignore instead.

alanhdu · 2023-06-01T20:12:48Z

fsspec/utils.py

@@ -500,7 +554,7 @@ def merge_offset_ranges(paths, starts, ends, max_gap=0, max_block=None, sort=Tru
    if not isinstance(starts, list):
        starts = [starts] * len(paths)
    if not isinstance(ends, list):
-        ends = [starts] * len(paths)
+        ends = [ends] * len(paths)


I assume this was just a bug? I can't figure out what this could would do otherwise.

@rjzamora ?

Certainly looks like a bug to me. That said, I don't think starts or ends ever correspond to anything other than list anyway. Perhaps we can just drop the int support since the result would have been "wrong" before anyway?

fsspec/caching.py

martindurant · 2023-06-03T20:19:59Z

I am waiting on this one hoping that anyone with more types experience than me can have a look. I don't see any glaring problems.

martindurant

Most of these are questions to educate myself.

fsspec/caching.py

martindurant · 2023-06-08T14:23:14Z

fsspec/utils.py

+    if length is not None:
+        b = f.read(length)
+    else:
+        b = f.read()


Let's not change the logic in this PR. I am not immediately sure about what you suggest.

fsspec/utils.py

martindurant · 2023-06-08T14:30:56Z

fsspec/utils.py

@@ -482,11 +530,18 @@ def wrapper(cls):


 @contextmanager
-def nullcontext(obj):
+def nullcontext(obj: T) -> Iterator[T]:


This is a generator before applying the decorator, but not a generator afterwards (it becomes a context). So, I'm not sure which state should be declared here.

Yeah it's a little confusing. This is the correct because contextmanager has a type annotation that turns the iterator into a context manager for you.

martindurant · 2023-06-08T14:32:43Z

fsspec/utils.py

@@ -500,7 +554,7 @@ def merge_offset_ranges(paths, starts, ends, max_gap=0, max_block=None, sort=Tru
    if not isinstance(starts, list):
        starts = [starts] * len(paths)
    if not isinstance(ends, list):
-        ends = [starts] * len(paths)
+        ends = [ends] * len(paths)


@rjzamora ?

martindurant

Most of these are questions to educate myself.

fsspec/utils.py

Skylion007 · 2023-09-05T15:20:57Z

fsspec/utils.py

@@ -500,7 +552,7 @@ def merge_offset_ranges(paths, starts, ends, max_gap=0, max_block=None, sort=Tru
    if not isinstance(starts, list):
        starts = [starts] * len(paths)
    if not isinstance(ends, list):
-        ends = [starts] * len(paths)
+        ends = [ends] * len(paths)
    if len(starts) != len(paths) or len(ends) != len(paths):
        raise ValueError


This seems like a bug, shouldn't we actually instatiate the error, and ideally print an error message about why it's thrown?

A message would be a reasonable thing to add here, but it's not necessary to instantiate if there is no message - python will do it automatically.

>>> raise ValueError Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError

Skylion007

I'm impressed, the typing in this PR is actually well/properly done. @martindurant since you wanted someone with more typing experience to take a look, this is actually pretty done well and except for minor comments all the typing annotations look correct.

Skylion007 · 2023-09-05T15:24:57Z

fsspec/utils.py

@@ -266,11 +295,11 @@ def read_block(f, offset, length, delimiter=None, split_before=False):
        length = end - start

    f.seek(offset)
-    b = f.read(length)
+    b = f.read(length)  # type: ignore[arg-type]


This doesn't seem like a typing error here. When can length ever be None and not cause an error (unless delimiter is not None). Instead of adding a type ignore comment, do an assert length is not None above it (unless f.read can actually take None and it's just a type stubbing problem?)

Maybe even go further and raise a ValueError if length is None and delimiter is None that they cannot both be None at the same time.

I added an assert here, but I wonder if we should to what I originally suggested of:

if length is None: b = f.read() else: b = f.read(length)

The docs say that if length is None we want to read until the end of the string, which I believe this will.

unless f.read can actually take None and it's just a type stubbing problem?

Arguably it is? I did just test with an actual file and confirm that open(file).read(None) gives reasonable results.

martindurant · 2023-09-05T15:30:12Z

Thanks for having a look, @Skylion007 . @alanhdu , do you expect to have time to respond and fix the conflicts that have appeared since you submitted this?

martindurant · 2023-10-04T14:26:55Z

This work has gone stale - is there any chance of reviving it?

Co-authored-by: Martin Durant <[email protected]>

alanhdu · 2023-10-19T20:50:39Z

@martindurant @Skylion007 Sorry for the delay here. I rebased and things should look be ok (modulo the discussion on #1284 (comment)).

I'm ok adding the assert (or raise a more informative ValueError), but we could keep it as # type: ignore if we want to guarantee no change in the runtime behavior.

martindurant · 2023-10-20T15:32:38Z

One type error outstanding

fsspec/caching.py:101: error: Incompatible return value type (got "bytearray", expected "mmap")  [return-value]

alanhdu · 2023-10-20T16:26:44Z

Fixed!

alanhdu force-pushed the alan/mypy branch from 8803251 to 9dc6045 Compare June 1, 2023 20:16

alanhdu commented Jun 1, 2023

View reviewed changes

martindurant reviewed Jun 2, 2023

View reviewed changes

fsspec/caching.py Outdated Show resolved Hide resolved

martindurant reviewed Jun 8, 2023

View reviewed changes

martindurant reviewed Jun 24, 2023

View reviewed changes

fsspec/utils.py Outdated Show resolved Hide resolved

Skylion007 reviewed Sep 5, 2023

View reviewed changes

Skylion007 approved these changes Sep 5, 2023

View reviewed changes

alanhdu and others added 4 commits October 17, 2023 23:53

Add typehints to fsspec.caching

a8034d3

Add typehints to fsspec.utils

a00afa9

Stricter type-checking for fsspec.caching and fsspec.utils

322bf74

Apply suggestions from code review

18f99f0

Co-authored-by: Martin Durant <[email protected]>

alanhdu force-pushed the alan/mypy branch from bb54b4c to 18f99f0 Compare October 18, 2023 03:54

alanhdu and others added 2 commits October 17, 2023 23:57

Add assert instead of type-ignore

a997f40

Apply suggestions from code review

bd6a36f

Co-authored-by: Martin Durant <[email protected]>

Fix mypy error

add6e23

martindurant merged commit dbad63f into fsspec:master Oct 20, 2023

alanhdu deleted the alan/mypy branch October 20, 2023 17:10

alanhdu mentioned this pull request Oct 20, 2023

Add typehints to fsspec.mapping and fsspec.dircache #1396

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typehints for `fsspec.caching` and `fsspec.utils` #1284

Add typehints for `fsspec.caching` and `fsspec.utils` #1284

alanhdu commented Jun 1, 2023

alanhdu Jun 1, 2023

martindurant Jun 8, 2023

alanhdu Jun 8, 2023

alanhdu Jun 1, 2023

martindurant Jun 8, 2023

rjzamora Jun 8, 2023

martindurant commented Jun 3, 2023

martindurant left a comment

martindurant Jun 8, 2023

martindurant Jun 8, 2023

alanhdu Jun 8, 2023

martindurant Jun 8, 2023

martindurant left a comment

Skylion007 Sep 5, 2023

martindurant Sep 5, 2023

Skylion007 left a comment

Skylion007 Sep 5, 2023 •

edited

Loading

Skylion007 Sep 5, 2023 •

edited

Loading

alanhdu Oct 19, 2023

martindurant commented Sep 5, 2023

martindurant commented Oct 4, 2023

alanhdu commented Oct 19, 2023

martindurant commented Oct 20, 2023

alanhdu commented Oct 20, 2023

Add typehints for fsspec.caching and fsspec.utils #1284

Add typehints for fsspec.caching and fsspec.utils #1284

Conversation

alanhdu commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martindurant commented Jun 3, 2023

martindurant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martindurant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skylion007 left a comment

Choose a reason for hiding this comment

Skylion007 Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Skylion007 Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martindurant commented Sep 5, 2023

martindurant commented Oct 4, 2023

alanhdu commented Oct 19, 2023

martindurant commented Oct 20, 2023

alanhdu commented Oct 20, 2023

Add typehints for `fsspec.caching` and `fsspec.utils` #1284

Add typehints for `fsspec.caching` and `fsspec.utils` #1284

Skylion007 Sep 5, 2023 •

edited

Loading

Skylion007 Sep 5, 2023 •

edited

Loading