| Message ID | 20230315105055.9b2be0153625.I7a2cb99b95dff216c0feed4604255275e0b156a7@changeid (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: tool: fix type annotation for IO | expand |
On Wed, Mar 15, 2023 at 10:57 PM Johannes Berg <johannes@sipsolutions.net> wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > This should be IO[str], since we use it for printing strings. This is a good catch, thanks. But we also have a few more bare generic types that warrant attention. I think the diff below [1] should fix the others as well. I can send it out as a formal patch and add your name for the Reported-by? [1] diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 53e90c335834..e6fc8fcb071a 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -92,7 +92,7 @@ class LinuxSourceTreeOperations: if stderr: # likely only due to build warnings print(stderr.decode()) - def start(self, params: List[str], build_dir: str) -> subprocess.Popen: + def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]: raise RuntimeError('not implemented!') @@ -112,7 +112,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations): kconfig.merge_in_entries(base_kunitconfig) return kconfig - def start(self, params: List[str], build_dir: str) -> subprocess.Popen: + def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]: kernel_path = os.path.join(build_dir, self._kernel_path) qemu_command = ['qemu-system-' + self._qemu_arch, '-nodefaults', @@ -141,7 +141,7 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations): kconfig.merge_in_entries(base_kunitconfig) return kconfig - def start(self, params: List[str], build_dir: str) -> subprocess.Popen: + def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]: """Runs the Linux UML binary. Must be named 'linux'.""" linux_bin = os.path.join(build_dir, 'linux') params.extend(['mem=1G', 'console=tty', 'kunit_shutdown=halt']) diff --git a/tools/testing/kunit/kunit_printer.py b/tools/testing/kunit/kunit_printer.py index 5f1cc55ecdf5..015adf87dc2c 100644 --- a/tools/testing/kunit/kunit_printer.py +++ b/tools/testing/kunit/kunit_printer.py @@ -15,7 +15,7 @@ _RESET = '\033[0;0m' class Printer: """Wraps a file object, providing utilities for coloring output, etc.""" - def __init__(self, output: typing.IO): + def __init__(self, output: typing.IO[str]): self._output = output self._use_color = output.isatty() diff --git a/tools/testing/kunit/run_checks.py b/tools/testing/kunit/run_checks.py index 066e6f938f6d..61cece1684df 100755 --- a/tools/testing/kunit/run_checks.py +++ b/tools/testing/kunit/run_checks.py @@ -37,7 +37,7 @@ def main(argv: Sequence[str]) -> None: if argv: raise RuntimeError('This script takes no arguments') - future_to_name: Dict[futures.Future, str] = {} + future_to_name: Dict[futures.Future[None], str] = {} executor = futures.ThreadPoolExecutor(max_workers=len(commands)) for name, argv in commands.items(): if name in necessary_deps and shutil.which(necessary_deps[name]) is None:
On Wed, 2023-03-15 at 23:02 -0700, Daniel Latypov wrote: > On Wed, Mar 15, 2023 at 10:57 PM Johannes Berg > <johannes@sipsolutions.net> wrote: > > > > From: Johannes Berg <johannes.berg@intel.com> > > > > This should be IO[str], since we use it for printing strings. > > This is a good catch, thanks. > But we also have a few more bare generic types that warrant attention. Oh, that might well be true. I was using kunit_parser in a script, and that imports kunit_printer, and then tried to check *my* script for type annotations with mypy. This led it to go check through the dependencies too, and since it was just one small thing there I decided to just fix it rather than figure out how to tell mypy that I don't care about those dependencies :-) Now for everything else? I didn't even look. > I think the diff below [1] should fix the others as well. > I can send it out as a formal patch and add your name for the Reported-by? Sure! johannes
On Thu, Mar 16, 2023 at 12:42 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Wed, 2023-03-15 at 23:02 -0700, Daniel Latypov wrote: > > This is a good catch, thanks. > > But we also have a few more bare generic types that warrant attention. > > Oh, that might well be true. I was using kunit_parser in a script, and > that imports kunit_printer, and then tried to check *my* script for type > annotations with mypy. This led it to go check through the dependencies > too, and since it was just one small thing there I decided to just fix > it rather than figure out how to tell mypy that I don't care about those > dependencies :-) There's --exclude='<regex>', if you ever end up needing to ignore other files. But yeah, we should try and make sure that mpyy is happy w/ kunit.py code. Thanks for doing this! > > Now for everything else? I didn't even look. Oh, does mypy complain about this now? That'd be nice. Hmm, I don't see it even after upgrading my local version. $ pip install --upgrade mypy pytype $ ../tools/testing/kunit/run_checks.py <no errors> # Checking if it doesn't report error but logs a warning: $ mypy ./tools/testing/kunit/*.py Success: no issues found in 9 source files How I found the rest is Google has a wrapper around pylint, which has a number of custom checks. "g-bare-generic" is one of them, which complains about these. I don't think there's a publicly accessible version of those checks, even though the base pylintrc file is... Note: I'd like it if kunit/run_checks.py could run pylint. But the default python style contrasts with kernel style (mainly around whitespace), so there's a lot of checks we'd need to disable to get a clean signal. > > > I think the diff below [1] should fix the others as well. > > I can send it out as a formal patch and add your name for the Reported-by? > > Sure! https://lore.kernel.org/linux-kselftest/20230316172900.755430-1-dlatypov@google.com Daniel
On Thu, 2023-03-16 at 10:35 -0700, Daniel Latypov wrote: > On Thu, Mar 16, 2023 at 12:42 AM Johannes Berg > <johannes@sipsolutions.net> wrote: > > > > On Wed, 2023-03-15 at 23:02 -0700, Daniel Latypov wrote: > > > This is a good catch, thanks. > > > But we also have a few more bare generic types that warrant attention. > > > > Oh, that might well be true. I was using kunit_parser in a script, and > > that imports kunit_printer, and then tried to check *my* script for type > > annotations with mypy. This led it to go check through the dependencies > > too, and since it was just one small thing there I decided to just fix > > it rather than figure out how to tell mypy that I don't care about those > > dependencies :-) > > There's --exclude='<regex>', if you ever end up needing to ignore other files. > But yeah, we should try and make sure that mpyy is happy w/ kunit.py code. Yes, but that has other complexities I think? Anyway, I didn't even really read much into it since it was so easy. > > Now for everything else? I didn't even look. > > Oh, does mypy complain about this now? > That'd be nice. > > Hmm, I don't see it even after upgrading my local version. > $ pip install --upgrade mypy pytype > $ ../tools/testing/kunit/run_checks.py > <no errors> > # Checking if it doesn't report error but logs a warning: > $ mypy ./tools/testing/kunit/*.py > Success: no issues found in 9 source files Oh, I use --strict, and $ mypy --version mypy 0.941 right now. Probably old. And $ mypy --strict tools/testing/kunit/*.py has a _lot_ of complaints. > How I found the rest is Google has a wrapper around pylint, which has > a number of custom checks. "g-bare-generic" is one of them, which > complains about these. > > I don't think there's a publicly accessible version of those checks, > even though the base pylintrc file is... :) > https://lore.kernel.org/linux-kselftest/20230316172900.755430-1-dlatypov@google.com > Great, thanks! johannes
On Thu, Mar 16, 2023 at 10:43 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Thu, 2023-03-16 at 10:35 -0700, Daniel Latypov wrote: > > On Thu, Mar 16, 2023 at 12:42 AM Johannes Berg > > <johannes@sipsolutions.net> wrote: > > > > > > On Wed, 2023-03-15 at 23:02 -0700, Daniel Latypov wrote: > > > > This is a good catch, thanks. > > > > But we also have a few more bare generic types that warrant attention. > > > > > > Oh, that might well be true. I was using kunit_parser in a script, and > > > that imports kunit_printer, and then tried to check *my* script for type > > > annotations with mypy. This led it to go check through the dependencies > > > too, and since it was just one small thing there I decided to just fix > > > it rather than figure out how to tell mypy that I don't care about those > > > dependencies :-) > > > > There's --exclude='<regex>', if you ever end up needing to ignore other files. > > But yeah, we should try and make sure that mpyy is happy w/ kunit.py code. > > Yes, but that has other complexities I think? Anyway, I didn't even > really read much into it since it was so easy. > > > > Now for everything else? I didn't even look. > > > > Oh, does mypy complain about this now? > > That'd be nice. > > > > Hmm, I don't see it even after upgrading my local version. > > $ pip install --upgrade mypy pytype > > $ ../tools/testing/kunit/run_checks.py > > <no errors> > > # Checking if it doesn't report error but logs a warning: > > $ mypy ./tools/testing/kunit/*.py > > Success: no issues found in 9 source files > > Oh, I use --strict, and > > $ mypy --version > mypy 0.941 > > right now. Probably old. > > And > > $ mypy --strict tools/testing/kunit/*.py > > has a _lot_ of complaints. Yikes. I either forgot or never knew about --strict, thanks for pointing it out. Looking into it, I don't think it's worth using for kunit.py, details below. With mypy 1.1.1, I get Found 172 errors in 5 files (checked 9 source files) Counting the different types ... | grep -Po '\[[a-z-]+\]$' | sort | uniq -c | sort -nr 108 [no-untyped-def] 61 [no-untyped-call] 1 [attr-defined] 1 [assignment] Hmm, the untyped stuff is not too important since we also use pytype, which is a lot smarter in that it infers types where not specified. The other two though warrant some attention * attr-defined: we don't care about this. We have a directive to squash this "error" for pytype. Ugh, do we need another one for pytype? * assignment: not actually a real error, it points to our type annotations being a bit sloppy tools/testing/kunit/kunit.py:459: error: Incompatible types in assignment (expression has type "List[str]", variable has type "TextIO") [assignment] So it doesn't like that we assign a list[str] or a IO[str] to `kunit_output`. But we're passing it in as Iterable[str], which both work as. Even explicitly annotating it as an Iterable[str] doesn't make it happy [1]. I'll ignore this, then. Daniel [1] diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 52853634ba23..a9f1146bcdce 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -453,10 +453,10 @@ def exec_handler(cli_args): def parse_handler(cli_args): if cli_args.file is None: sys.stdin.reconfigure(errors='backslashreplace') # pytype: disable=attribute-error - kunit_output = sys.stdin + kunit_output = sys.stdin # type[Iterable[str]] else: with open(cli_args.file, 'r', errors='backslashreplace') as f: - kunit_output = f.read().splitlines() + kunit_output = f.read().splitlines() # type[Iterable[str]] # We know nothing about how the result was created! metadata = kunit_json.Metadata() request = KunitParseRequest(raw_output=cli_args.raw_output,
On Thu, 2023-03-16 at 10:58 -0700, Daniel Latypov wrote: > Yikes. > I either forgot or never knew about --strict, thanks for pointing it out. > > Looking into it, I don't think it's worth using for kunit.py, details below. Oh, no argument, whatever works for you. I don't really even remember why I/we settled on mypy vs. pytypes, I don't remember really doing any comparison. I do find the whole type-checking, if occasionally a bit painful, to be of benefit though, at least for larger code bases (and by larger I think I probably would say "more than a single file that you can keep in your head entirely" :-) > Hmm, the untyped stuff is not too important since we also use pytype, > which is a lot smarter in that it infers types where not specified. Hmm, mypy normally does a bunch of inference too, but yeah might be less smart here, or maybe --strict just insists on more precision. > The other two though warrant some attention > * attr-defined: we don't care about this. We have a directive to > squash this "error" for pytype. Ugh, do we need another one for > pytype? You mean mypy? I guess yes, you'd need "# type: ignore". But funnily, I think both tools will insist that you place the comment on the same line as the error, and that doesn't really work since you can only have one comment on that line :-) However, it looks like pytype would also accept "# type: ignore" according to the docs. > * assignment: not actually a real error, it points to our type > annotations being a bit sloppy Not sure what you meant by this, but maybe I just don't see it with my version of mypy. > tools/testing/kunit/kunit.py:459: error: Incompatible types in > assignment (expression has type "List[str]", variable has type > "TextIO") [assignment] > > So it doesn't like that we assign a list[str] or a IO[str] to `kunit_output`. > But we're passing it in as Iterable[str], which both work as. > > Even explicitly annotating it as an Iterable[str] doesn't make it happy [1]. > I'll ignore this, then. I think you got that annotation wrong, at least as far as mypy is concerned? > sys.stdin.reconfigure(errors='backslashreplace') #pytype: disable=attribute-error > - kunit_output = sys.stdin > + kunit_output = sys.stdin # type[Iterable[str]] Doing kunit_output: Iterable[str] = sys.stdin actually fixes it for me, and you don't even need the second one: > else: > with open(cli_args.file, 'r', errors='backslashreplace') as f: > - kunit_output = f.read().splitlines() > + kunit_output = f.read().splitlines() #type[Iterable[str]] > which I think is because it assigned the type as Iterable[str] after the first one, and then was happy since splitlines() is Iterable[str] after all. So maybe pytypes is smarter about finding the best common type. Anyway ... I don't really care so much. I just noticed this, and had it been any more complex I'd probably have gone and just ignored kunit from my project. Oh, another unrelated thing: it kind of bothered me that even with kunit_parser.py you automatically get some output on stdout, it felt like kunit_parser.py was or should be more of a library, but in the end I don't really care so much since I needed to write a JSON file for our internal system and could just suppress the stdout entirely :) johannes
On Thu, Mar 16, 2023 at 2:03 PM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Thu, 2023-03-16 at 10:58 -0700, Daniel Latypov wrote: > > Yikes. > > I either forgot or never knew about --strict, thanks for pointing it out. > > > > Looking into it, I don't think it's worth using for kunit.py, details below. > > Oh, no argument, whatever works for you. I don't really even remember > why I/we settled on mypy vs. pytypes, I don't remember really doing any > comparison. > > I do find the whole type-checking, if occasionally a bit painful, to be > of benefit though, at least for larger code bases (and by larger I think > I probably would say "more than a single file that you can keep in your > head entirely" :-) 100% agree, just the concern is about trying to keep two different checkers happy when they might have conflicting demands. Pytype has been able to catch some relatively subtle errors, so I've prioritized it. But since commit b7e0b983ff13 ("kunit: tool: fix pre-existing python type annotation errors") onwards, I've been trying to fix up all the issues we've had with either. > > > Hmm, the untyped stuff is not too important since we also use pytype, > > which is a lot smarter in that it infers types where not specified. > > Hmm, mypy normally does a bunch of inference too, but yeah might be less > smart here, or maybe --strict just insists on more precision. One footgun I ran into was that mypy seemed like it wasn't even touching functions if they didn't have an argument or the return type annotated. See 09641f7c7d8f ("kunit: tool: surface and address more typing issues") Maybe --strict is better. Pytype takes a lot longer and caught issues that normal `mypy` didn't. I haven't compared them w/ strict --strict. But given pytype is still a lot slower, I assume (hope) it's doing more work. > > > The other two though warrant some attention > > * attr-defined: we don't care about this. We have a directive to > > squash this "error" for pytype. Ugh, do we need another one for > > pytype? > > You mean mypy? I guess yes, you'd need "# type: ignore". But funnily, I Ah yes, I meant mypy. > think both tools will insist that you place the comment on the same line > as the error, and that doesn't really work since you can only have one > comment on that line :-) However, it looks like pytype would also accept > "# type: ignore" according to the docs. That's true, but `type: ignore` disables all type checking on the line. In this case, I think it's moot given the whole line is just sys.stdin.reconfigure(errors='backslashreplace') so I'd basically turned off all type-checking already. > > > > * assignment: not actually a real error, it points to our type > > annotations being a bit sloppy > > Not sure what you meant by this, but maybe I just don't see it with my > version of mypy. > > > tools/testing/kunit/kunit.py:459: error: Incompatible types in > > assignment (expression has type "List[str]", variable has type > > "TextIO") [assignment] > > > > So it doesn't like that we assign a list[str] or a IO[str] to `kunit_output`. > > But we're passing it in as Iterable[str], which both work as. > > > > Even explicitly annotating it as an Iterable[str] doesn't make it happy [1]. > > I'll ignore this, then. > > I think you got that annotation wrong, at least as far as mypy is > concerned? > > > sys.stdin.reconfigure(errors='backslashreplace') #pytype: disable=attribute-error > > - kunit_output = sys.stdin > > + kunit_output = sys.stdin # type[Iterable[str]] > > Doing > > kunit_output: Iterable[str] = sys.stdin > > actually fixes it for me, and you don't even need the second one: D'oh, I forgot the ": " Ugh, magic comments :) Doing "# type: Iterable[str]` on just the first one works for me. Thanks! Note: we'd been avoiding the `var: T = 42` syntax to try and be more compatible with old versions of python. But since we formally added a check that it's running python>3.7, I guess we can switch to that syntax now too. > > > else: > > with open(cli_args.file, 'r', errors='backslashreplace') as f: > > - kunit_output = f.read().splitlines() > > + kunit_output = f.read().splitlines() #type[Iterable[str]] > > > > which I think is because it assigned the type as Iterable[str] after the > first one, and then was happy since splitlines() is Iterable[str] after > all. So maybe pytypes is smarter about finding the best common type. > > > > Anyway ... I don't really care so much. I just noticed this, and had it > been any more complex I'd probably have gone and just ignored kunit from > my project. > > Oh, another unrelated thing: it kind of bothered me that even with > kunit_parser.py you automatically get some output on stdout, it felt > like kunit_parser.py was or should be more of a library, but in the end > I don't really care so much since I needed to write a JSON file for our > internal system and could just suppress the stdout entirely :) Ack, yeah, the fact it dumps anything directly to stdout is annoying. The argument is that * it wants to print in real time * it needs access to the tty to know if it should include colors or not This patch might be of interest to you: https://kunit-review.git.corp.google.com/c/linux/+/5730/1 With that, you could pass in a kunit_printer.BufferPrinter to parse_tests() and keep stdout pristine. I could split out the concurrency support from that patch and try to get submitted. There just wasn't motivation to do so since there was no use case for suppressing output yet. Given you're using it, that might be sufficient. Daniel
On Thu, 2023-03-16 at 14:30 -0700, Daniel Latypov wrote: > > 100% agree, just the concern is about trying to keep two different > checkers happy when they might have conflicting demands. > Pytype has been able to catch some relatively subtle errors, so I've > prioritized it. Sure. Maybe I'll just switch to pytype too ;-) > But since commit b7e0b983ff13 ("kunit: tool: fix pre-existing python > type annotation errors") onwards, I've been trying to fix up all the > issues we've had with either. OK. > One footgun I ran into was that mypy seemed like it wasn't even > touching functions if they didn't have an argument or the return type > annotated. > See 09641f7c7d8f ("kunit: tool: surface and address more typing issues") > Maybe --strict is better. It might not be, I have no idea. > Pytype takes a lot longer and caught issues that normal `mypy` didn't. > I haven't compared them w/ strict --strict. But given pytype is still > a lot slower, I assume (hope) it's doing more work. Hehe :-) > > think both tools will insist that you place the comment on the same line > > as the error, and that doesn't really work since you can only have one > > comment on that line :-) However, it looks like pytype would also accept > > "# type: ignore" according to the docs. > > That's true, but `type: ignore` disables all type checking on the line. > > In this case, I think it's moot given the whole line is just > sys.stdin.reconfigure(errors='backslashreplace') > so I'd basically turned off all type-checking already. Right, pytype looks a bit more controlled there. > > I think you got that annotation wrong, at least as far as mypy is > > concerned? > > > > > sys.stdin.reconfigure(errors='backslashreplace') #pytype: disable=attribute-error > > > - kunit_output = sys.stdin > > > + kunit_output = sys.stdin # type[Iterable[str]] > > > > Doing > > > > kunit_output: Iterable[str] = sys.stdin > > > > actually fixes it for me, and you don't even need the second one: > > D'oh, I forgot the ": " Ugh, magic comments :) > Doing "# type: Iterable[str]` on just the first one works for me. Right, not sure I like the magic comments better than the inline type in the code, but YMMV. > Note: we'd been avoiding the `var: T = 42` syntax to try and be more > compatible with old versions of python. Well, OK, I gave up on non-controlled versions of python long ago. In my case it's all running in a nix-shell environment so I control it very precisely :-) > Ack, yeah, the fact it dumps anything directly to stdout is annoying. > > The argument is that > * it wants to print in real time > * it needs access to the tty to know if it should include colors or not Right. > This patch might be of interest to you: > https://kunit-review.git.corp.google.com/c/linux/+/5730/1 > With that, you could pass in a kunit_printer.BufferPrinter to > parse_tests() and keep stdout pristine. Heh. Sounds like a useful patch but I can't see that link given the lack of corp.google.com login :P > I could split out the concurrency support from that patch and try to > get submitted. > There just wasn't motivation to do so since there was no use case for > suppressing output yet. > Given you're using it, that might be sufficient. I honestly don't care much now. Basically decided that redirecting stdout to /dev/null was sufficient, since anyway the real output I need happens to a file. I might've designed it to print the JSON on stdout instead if it wasn't getting all that output from kunit, but it really doesn't matter now. Anyway, thanks for the discussion! I've been playing with kunit only for like 48 hours now, so we'll see where I can go from here :-) johannes
On Thu, Mar 16, 2023 at 2:30 PM Daniel Latypov <dlatypov@google.com> wrote: > > I do find the whole type-checking, if occasionally a bit painful, to be > > of benefit though, at least for larger code bases (and by larger I think > > I probably would say "more than a single file that you can keep in your > > head entirely" :-) > > 100% agree, just the concern is about trying to keep two different > checkers happy when they might have conflicting demands. > Pytype has been able to catch some relatively subtle errors, so I've > prioritized it. Actually, I've gotten both pytype and `mypy --strict` happy now: https://lore.kernel.org/linux-kselftest/20230316220638.983743-3-dlatypov@google.com/ That patch also updates kunit/run_checks.py so hopefully we can keep `mypy --strict` happy going forward too. Here's to safer python, Daniel
diff --git a/tools/testing/kunit/kunit_printer.py b/tools/testing/kunit/kunit_printer.py index 5f1cc55ecdf5..015adf87dc2c 100644 --- a/tools/testing/kunit/kunit_printer.py +++ b/tools/testing/kunit/kunit_printer.py @@ -15,7 +15,7 @@ _RESET = '\033[0;0m' class Printer: """Wraps a file object, providing utilities for coloring output, etc.""" - def __init__(self, output: typing.IO): + def __init__(self, output: typing.IO[str]): self._output = output self._use_color = output.isatty()