diff mbox series

[v2,1/3] kunit: tool: add subscripts for type annotations where appropriate

Message ID 20230316220638.983743-1-dlatypov@google.com (mailing list archive)
State Accepted
Commit 695e26030858b27648ca107b77095fed53377b4b
Delegated to: Brendan Higgins
Headers show
Series [v2,1/3] kunit: tool: add subscripts for type annotations where appropriate | expand

Commit Message

Daniel Latypov March 16, 2023, 10:06 p.m. UTC 
E.g. for subprocess.Popen, it can be opened in `text=True` mode where it
returns strings, or `text=False` where it returns bytes.
To differentiate, you can annotate types as `Popen[str]` or
`Popen[bytes]`.

This patch should add subscripts in all the places we were missing them.

Reported-by: Johannes Berg <johannes.berg@intel.com>
Link: https://lore.kernel.org/linux-kselftest/20230315105055.9b2be0153625.I7a2cb99b95dff216c0feed4604255275e0b156a7@changeid/
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
Note: this is unchanged, just added a 3rd patch to this series.
---
 tools/testing/kunit/kunit_kernel.py  | 6 +++---
 tools/testing/kunit/kunit_printer.py | 2 +-
 tools/testing/kunit/run_checks.py    | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)


base-commit: 2c6a96dad5797e57b4cf04101d6c8d5c7a571603

Comments

David Gow March 17, 2023, 5:45 a.m. UTC | #1 
On Fri, 17 Mar 2023 at 06:06, Daniel Latypov <dlatypov@google.com> wrote:
>
> E.g. for subprocess.Popen, it can be opened in `text=True` mode where it
> returns strings, or `text=False` where it returns bytes.
> To differentiate, you can annotate types as `Popen[str]` or
> `Popen[bytes]`.
>
> This patch should add subscripts in all the places we were missing them.
>
> Reported-by: Johannes Berg <johannes.berg@intel.com>
> Link: https://lore.kernel.org/linux-kselftest/20230315105055.9b2be0153625.I7a2cb99b95dff216c0feed4604255275e0b156a7@changeid/
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
> Note: this is unchanged, just added a 3rd patch to this series.
> ---

Looks good.

Reviewed-by: David Gow <davidgow@google.com>

Thanks, Johannes and Daniel!

-- David

>  tools/testing/kunit/kunit_kernel.py  | 6 +++---
>  tools/testing/kunit/kunit_printer.py | 2 +-
>  tools/testing/kunit/run_checks.py    | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> 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:
>
> base-commit: 2c6a96dad5797e57b4cf04101d6c8d5c7a571603
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>
SeongJae Park April 30, 2023, 6:15 p.m. UTC | #2 
Hi Daniel,

On Thu, 16 Mar 2023 15:06:36 -0700 Daniel Latypov <dlatypov@google.com> wrote:

> E.g. for subprocess.Popen, it can be opened in `text=True` mode where it
> returns strings, or `text=False` where it returns bytes.
> To differentiate, you can annotate types as `Popen[str]` or
> `Popen[bytes]`.
> 
> This patch should add subscripts in all the places we were missing them.

I just found this patch is in the latest mainline tree, and it causes kunit
failure on my test machine like below.

    $ python3 --version
    Python 3.8.10
    $
    $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
    Traceback (most recent call last):
      File "./tools/testing/kunit/kunit.py", line 24, in <module>
        import kunit_kernel
      File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 42, in <module>
        class LinuxSourceTreeOperations:
      File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 95, in LinuxSourceTreeOperations
        def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
    TypeError: 'type' object is not subscriptable
    $

I further confirmed reverting this patch makes it run again.  Do you have any
idea?


Thanks,
SJ

> 
> Reported-by: Johannes Berg <johannes.berg@intel.com>
> Link: https://lore.kernel.org/linux-kselftest/20230315105055.9b2be0153625.I7a2cb99b95dff216c0feed4604255275e0b156a7@changeid/
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
> Note: this is unchanged, just added a 3rd patch to this series.
> ---
>  tools/testing/kunit/kunit_kernel.py  | 6 +++---
>  tools/testing/kunit/kunit_printer.py | 2 +-
>  tools/testing/kunit/run_checks.py    | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> 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:
> 
> base-commit: 2c6a96dad5797e57b4cf04101d6c8d5c7a571603
> -- 
> 2.40.0.rc1.284.g88254d51c5-goog
>
Daniel Latypov April 30, 2023, 9:34 p.m. UTC | #3 
On Sun, Apr 30, 2023 at 11:15 AM SeongJae Park <sj@kernel.org> wrote:
>
> Hi Daniel,
>
> On Thu, 16 Mar 2023 15:06:36 -0700 Daniel Latypov <dlatypov@google.com> wrote:
>
> > E.g. for subprocess.Popen, it can be opened in `text=True` mode where it
> > returns strings, or `text=False` where it returns bytes.
> > To differentiate, you can annotate types as `Popen[str]` or
> > `Popen[bytes]`.
> >
> > This patch should add subscripts in all the places we were missing them.
>
> I just found this patch is in the latest mainline tree, and it causes kunit
> failure on my test machine like below.
>
>     $ python3 --version
>     Python 3.8.10
>     $
>     $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
>     Traceback (most recent call last):
>       File "./tools/testing/kunit/kunit.py", line 24, in <module>
>         import kunit_kernel
>       File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 42, in <module>
>         class LinuxSourceTreeOperations:
>       File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 95, in LinuxSourceTreeOperations
>         def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
>     TypeError: 'type' object is not subscriptable
>     $
>
> I further confirmed reverting this patch makes it run again.  Do you have any
> idea?

It seems like support for the subscript wasn't added until Python 3.9.

I know support for subscripting other types like re.Pattern was added
in 3.9 per https://peps.python.org/pep-0585/ but it doesn't mention
Popen there...
This patch also added typing.IO[str] and concurrent.Future[None], so
those might be problematic too.

Can you check if the typing.IO and concurrent.Future[None] changes
cause problems?
(I don't have an easy way of testing against older Python versions currently).

If so, we should revert the patch.
If not, we can undo just the Popen changes.

And in either case, we'll need to update ./tools/testing/kunit/run_checks.py.
Currently, it runs `mypy --strict` which will start failing if we
revert any part of this patch.

Thanks,
Daniel
SeongJae Park May 1, 2023, 5:15 p.m. UTC | #4 
Hi Daniel,

On Sun, 30 Apr 2023 14:34:09 -0700 Daniel Latypov <dlatypov@google.com> wrote:

> On Sun, Apr 30, 2023 at 11:15 AM SeongJae Park <sj@kernel.org> wrote:
> >
> > Hi Daniel,
> >
> > On Thu, 16 Mar 2023 15:06:36 -0700 Daniel Latypov <dlatypov@google.com> wrote:
> >
> > > E.g. for subprocess.Popen, it can be opened in `text=True` mode where it
> > > returns strings, or `text=False` where it returns bytes.
> > > To differentiate, you can annotate types as `Popen[str]` or
> > > `Popen[bytes]`.
> > >
> > > This patch should add subscripts in all the places we were missing them.
> >
> > I just found this patch is in the latest mainline tree, and it causes kunit
> > failure on my test machine like below.
> >
> >     $ python3 --version
> >     Python 3.8.10
> >     $
> >     $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> >     Traceback (most recent call last):
> >       File "./tools/testing/kunit/kunit.py", line 24, in <module>
> >         import kunit_kernel
> >       File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 42, in <module>
> >         class LinuxSourceTreeOperations:
> >       File "/home/sjpark/linux/tools/testing/kunit/kunit_kernel.py", line 95, in LinuxSourceTreeOperations
> >         def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
> >     TypeError: 'type' object is not subscriptable
> >     $
> >
> > I further confirmed reverting this patch makes it run again.  Do you have any
> > idea?
> 
> It seems like support for the subscript wasn't added until Python 3.9.
> 
> I know support for subscripting other types like re.Pattern was added
> in 3.9 per https://peps.python.org/pep-0585/ but it doesn't mention
> Popen there...
> This patch also added typing.IO[str] and concurrent.Future[None], so
> those might be problematic too.
> 
> Can you check if the typing.IO and concurrent.Future[None] changes
> cause problems?
> (I don't have an easy way of testing against older Python versions currently).

Thank you for quick reply.  Reverting Popen changes only as below fixed my
issue.  So seems typing.IO and concurrent.Future[None] chages doesn't cause
problems at least for my use case.

    diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
    index f01f94106129..7f648802caf6 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[str]:
    +	def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
     		raise RuntimeError('not implemented!')
     
     
    @@ -113,7 +113,7 @@ class LinuxSourceTreeOperationsQemu(LinuxSourceTreeOperations):
     		kconfig.merge_in_entries(base_kunitconfig)
     		return kconfig
     
    -	def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
    +	def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
     		kernel_path = os.path.join(build_dir, self._kernel_path)
     		qemu_command = ['qemu-system-' + self._qemu_arch,
     				'-nodefaults',
    @@ -142,7 +142,7 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
     		kconfig.merge_in_entries(base_kunitconfig)
     		return kconfig
     
    -	def start(self, params: List[str], build_dir: str) -> subprocess.Popen[str]:
    +	def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
     		"""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'])

> 
> If so, we should revert the patch.
> If not, we can undo just the Popen changes.
> 
> And in either case, we'll need to update ./tools/testing/kunit/run_checks.py.
> Currently, it runs `mypy --strict` which will start failing if we
> revert any part of this patch.

Those make sense.


Thanks,
SJ

> 
> Thanks,
> Daniel
Daniel Latypov May 1, 2023, 6:19 p.m. UTC | #5 
On Mon, May 1, 2023 at 10:15 AM SeongJae Park <sj@kernel.org> wrote:
>

[snip]

> >
> > It seems like support for the subscript wasn't added until Python 3.9.
> >
> > I know support for subscripting other types like re.Pattern was added
> > in 3.9 per https://peps.python.org/pep-0585/ but it doesn't mention
> > Popen there...
> > This patch also added typing.IO[str] and concurrent.Future[None], so
> > those might be problematic too.
> >
> > Can you check if the typing.IO and concurrent.Future[None] changes
> > cause problems?
> > (I don't have an easy way of testing against older Python versions currently).
>
> Thank you for quick reply.  Reverting Popen changes only as below fixed my
> issue.  So seems typing.IO and concurrent.Future[None] chages doesn't cause
> problems at least for my use case.

Sounds good.
Sent https://lore.kernel.org/linux-kselftest/20230501181610.2617032-1-dlatypov@google.com

I was hoping adding `--python-version 3.7` to the `mypy --strict`
invocation would help, but it still fails for me :\

$ ./tools/testing/kunit/run_cheks.py
...
mypy: FAILED
> kunit_kernel.py:95: error: Missing type parameters for generic type "Popen"  [type-arg]
> kunit_kernel.py:116: error: Missing type parameters for generic type "Popen"  [type-arg]
> kunit_kernel.py:145: error: Missing type parameters for generic type "Popen"  [type-arg]
> Found 3 errors in 1 file (checked 8 source files)

And here I was, hoping it would complain about code incompatible with
python 3.7...
But at the very least, that patch should fix the current problem.

Daniel
diff mbox series

Patch

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: