CI test-source jobs failing since Jan 29

[victorlin]

Error is:

/home/runner/work/cli/cli/nextstrain/cli/command/view.py:390:14 - error: Argument of type "tuple[str, int] | tuple[str, int, int, int] | tuple[int, bytes]" cannot be assigned to parameter "sockaddr" of type "Tuple[str, int] | Tuple[str, int, int, int]" in function "__new__"

----------------------------- Captured stdout call -----------------------------
https://nextstrain.org/
/home/runner/work/cli/cli/nextstrain/cli/net.py
  /home/runner/work/cli/cli/nextstrain/cli/net.py:36:32 - error: Argument of type "tuple[str, int] | tuple[str, int, int, int] | tuple[int, bytes]" cannot be assigned to parameter "sockaddr" of type "tuple[str, int] | tuple[str, int, int, int]" in function "getnameinfo"
    Type "tuple[str, int] | tuple[str, int, int, int] | tuple[int, bytes]" is not assignable to type "tuple[str, int] | tuple[str, int, int, int]"
      Type "tuple[int, bytes]" is not assignable to type "tuple[str, int] | tuple[str, int, int, int]"
        "tuple[int, bytes]" is not assignable to "tuple[str, int]"
          Tuple entry 1 is incorrect type
            "int" is not assignable to "str"
        "tuple[int, bytes]" is not assignable to "tuple[str, int, int, int]"
          Tuple size mismatch; expected 4 but received 2 (reportArgumentType)
/home/runner/work/cli/cli/nextstrain/cli/command/view.py
  /home/runner/work/cli/cli/nextstrain/cli/command/view.py:390:14 - error: Argument of type "tuple[str, int] | tuple[str, int, int, int] | tuple[int, bytes]" cannot be assigned to parameter "sockaddr" of type "Tuple[str, int] | Tuple[str, int, int, int]" in function "__new__"
    Type "tuple[str, int] | tuple[str, int, int, int] | tuple[int, bytes]" is not assignable to type "Tuple[str, int] | Tuple[str, int, int, int]"
      Type "tuple[int, bytes]" is not assignable to type "Tuple[str, int] | Tuple[str, int, int, int]"
        "tuple[int, bytes]" is not assignable to "Tuple[str, int]"
          Tuple entry 1 is incorrect type
            "int" is not assignable to "str"
        "tuple[int, bytes]" is not assignable to "Tuple[str, int, int, int]"
          Tuple size mismatch; expected 4 but received 2 (reportArgumentType)
2 errors, 0 warnings, 0 informations

Example of last good run and first bad run.

[tsibley]

I noted this failure when it first started—I typically get alerts of scheduled CI failures here—and figured I'd fix it eventually. But feel free to beat me to it! It looked like another typeshed change/refinement that makes our unchanged code "wrong" in the eyes of pyright.

[victorlin]

Thanks! I may look into this today.

I typically get alerts of scheduled CI failures here

I assume you get these because you're the last person to modify that workflow? Is it worth adding issue automation instead?

[victorlin]

I dug into the Pyright source code and found that microsoft/pyright@3089c82 / python/typeshed@3de624f are the upstream changes that resulted in the type errors we're seeing. The changes add tuple[int, bytes] to the possible return types of getaddrinfo. From what I can tell based on linked issues, it seems pretty edge-case-y as tuple[int, bytes] is only returned when Python is compiled with --disable-ipv6.

We have two type errors:

1. nextstrain/cli/net.py:36:32

   cli/nextstrain/cli/net.py

   Line 36 in 4a00d71

   ip_address(getnameinfo(sockaddr, NI_NUMERICHOST)[0])

   I think this should be fixed upstream: Socket address type should be updated elsewhere python/typeshed#13472

2. nextstrain/cli/command/view.py:390:14

   cli/nextstrain/cli/command/view.py

   Line 390 in 4a00d71

   addrs = [AddressInfo(*a) for a in getaddrinfo(host, port, proto = IPPROTO_TCP)]

   For this one, we may need to add tuple[int, bytes] to the following type annotation:

   cli/nextstrain/cli/command/view.py

   Line 405 in 4a00d71

   sockaddr: Union[Tuple[str, int], Tuple[str, int, int, int]] # (ip, addr, ...)

   but then that surfaces another type error in that usage of sockaddr[0] as int and sockaddr[1] as bytes isn't valid here:

   cli/nextstrain/cli/command/view.py

   Lines 395 to 397 in 4a00d71

   	return (ip4[0].sockaddr[0], ip4[0].sockaddr[1]) if ip4 \
   	else (ip6[0].sockaddr[0], ip6[0].sockaddr[1]) if ip6 \
   	else (str(host), int(port))

   I wonder if we need to support Python compiled with --disable-ipv6. Can we throw a runtime error in such case to narrow types down to what's currently expected?

[tsibley]

I assume you get these because you're the last person to modify that workflow? Is it worth adding issue automation instead?

Correct. Yes, I think worth adding, esp. if others (like you! :-) are inclined to address CI failures too. That said, scheduled CI has been having a rough go of it recently, due to what I think are filesystem issues, so any automation is likely to cause some flapping of issues.

2. I wonder if we need to support Python compiled with --disable-ipv6. Can we throw a runtime error in such case to narrow types down to what's currently expected?

I don't know if we need to support it. No idea how prevalent that build flag is. But I don't think it would be hard to support, e.g.

     ip4 = [a for a in addrs if a.family is AF_INET]
-    ip6 = [a for a in addrs if a.family is AF_INET6]
+    ip6 = [a for a in addrs if a.family is AF_INET6 and isinstance(a.sockaddr[1], int)]
 
     return (ip4[0].sockaddr[0], ip4[0].sockaddr[1]) if ip4 \
       else (ip6[0].sockaddr[0], ip6[0].sockaddr[1]) if ip6 \
       else (str(host), int(port))

and so I would tend to think we should.

[victorlin]

Hmm interesting. I don't know what tuple[int, bytes] represents (forthcoming docs only mention that it's "raw data"), but my guess is that we can't resolve an address and port number from that information. Your suggestion to fall back to (str(host), int(port)) seems reasonable.

The one line change above still results in type errors because it's the return statement that's problematic for type checking – sockaddr[1] can still be bytes. This works and I think is functionally identical:

-    ip4 = [a for a in addrs if a.family is AF_INET]
-    ip6 = [a for a in addrs if a.family is AF_INET6]
-
-    return (ip4[0].sockaddr[0], ip4[0].sockaddr[1]) if ip4 \
-      else (ip6[0].sockaddr[0], ip6[0].sockaddr[1]) if ip6 \
-      else (str(host), int(port))
+    for a in addrs:
+        if a.family is AF_INET or a.family is AF_INET6:
+            assert (isinstance(a.sockaddr[0], str)
+                and isinstance(a.sockaddr[1], int))
+            return (a.sockaddr[0], a.sockaddr[1])
+        else:
+            return (str(host), int(port))

(re: issue automation: cool, I'll add it then! We can always disable the workflow if the flappy issues become annoying.)

[tsibley]

The one line change above still results in type errors because it's the return statement that's problematic for type checking – sockaddr[1] can still be bytes. This works and I think is functionally identical:

Then type checking is not narrowing correctly due to the list comprehension? Or I guess maybe it needs the and isinstance(…) check added to the ip4 comprehension too since it doesn't know that tuple[str, bytes] is limited to IPv6?

I don't think it's functionally identical since that fails an assertion when IPv6 is disabled in Python instead of merely skipping IPv6.

[victorlin]

I don't think it's functionally identical since that fails an assertion when IPv6 is disabled in Python instead of merely skipping IPv6.

Sorry, I wasn't thinking clearly when posting that patch. It's also wrong in that it returns prematurely and doesn't prioritize IPv4. What I was going for is this, which should be functionally identical:

     ip4 = [a for a in addrs if a.family is AF_INET]
     ip6 = [a for a in addrs if a.family is AF_INET6]
 
-    return (ip4[0].sockaddr[0], ip4[0].sockaddr[1]) if ip4 \
-      else (ip6[0].sockaddr[0], ip6[0].sockaddr[1]) if ip6 \
-      else (str(host), int(port))
+    for a in ip4:
+        if isinstance(a.sockaddr[0], str) and isinstance(a.sockaddr[1], int):
+            return (a.sockaddr[0], a.sockaddr[1])
+
+    for a in ip6:
+        if isinstance(a.sockaddr[0], str) and isinstance(a.sockaddr[1], int):
+            return (a.sockaddr[0], a.sockaddr[1])
+
+    return str(host), int(port)

Then type checking is not narrowing correctly due to the list comprehension? Or I guess maybe it needs the and isinstance(…) check added to the ip4 comprehension too since it doesn't know that tuple[str, bytes] is limited to IPv6?

Yes, and sockaddr[0] also needs to be checked. I think you were going for this:

-    ip4 = [a for a in addrs if a.family is AF_INET]
-    ip6 = [a for a in addrs if a.family is AF_INET6]
+    ip4 = [a for a in addrs if a.family is AF_INET and isinstance(a.sockaddr[0], str) and isinstance(a.sockaddr[1], int)]
+    ip6 = [a for a in addrs if a.family is AF_INET6 and isinstance(a.sockaddr[0], str) and isinstance(a.sockaddr[1], int)]

but unfortunately the narrowing from the list comprehension doesn't make it to the return line, which refers to the type annotation for AddressInfo.sockaddr.

[tsibley]

Yes, and sockaddr[0] also needs to be checked. I think you were going for this:

Well, but sockaddr[0] is only not str when sockaddr[1] is bytes, right? And while the signature type is non-specific about IPv4 vs. IPv6, the comment/doc is clear that this new tuple[int, bytes] value is only for IPv6.

I was going for supporting the new return value (i.e. gracefully handling it), not making the type checking work out.

IMO we're contorting this code for the type checker and we shouldn't.

[victorlin]

Thanks for the sanity check. I've tweaked your original patch and opened a PR: #414