Weaken type check on Result.load() return type #2196

schaefi · 2022-10-15T15:14:57Z

With an update of mypy the bound TypeVar is no longer allowed. To be honest I did not get a clue on the error message and opened this pull request which basically removes the type check for the return type of the load() method. I'm happy if we find a better solution during review. Thanks

jesusbv · 2022-10-17T11:26:29Z

kiwi/system/result.py

@@ -162,7 +160,7 @@ def dump(self, filename: str, portable: bool = True) -> None:
            )

    @staticmethod
-    def load(filename: str) -> result_type:


I get error: A function returning TypeVar should receive at least one argument containing the same Typevar
This issue AFAIU comes from this check, hope that clarifies the error message
IMO, the solution could be to return class type Result and remove the result_type definition as it is not needed now

def load(filename: str) -> Result:

That I tried which results in

check run-test: commands[0] | flake8 --statistics -j auto --count /home/ms/Project/kiwi/kiwi kiwi/system/result.py:165:32: F821 undefined name 'Result' 1 F821 undefined name 'Result' 1

which was the reason for using

result_type = TypeVar('result_type', bound='Result')

Yes, but that is fixable adding

from __future__ import annotations

at the beggining of the file (more info here, notice It will become the default in Python 3.10. in that doc)
or

def load(filename: str) -> Result: # noqa: F821

Now, it is a decision of

do we want to keep Result (type method returns) ? My answer is yes

do we want to add #noqa ? My opinion is no, because that would hide possible errors but no strong opinion

do we want to remove the type ? I think we should keep it

my 2 cents

Yes, but that is fixable adding

This is true beginning with python 3.7, but __future__ was not backported to 3.6 ... but 3.6 is still the default
on SLE15 / Leap15 which is still a supported platform

Oh man it could have been so easy :)

We could just make SUSE build this against a newer Python in SLE 15. They have Python 3.6, 3.8, and 3.10 now. I guess for RHEL 8, I'd have to do the same, which would not be fun to do...

jesusbv · 2022-10-18T12:36:20Z

kiwi/system/result.py

 from typing import (
    Dict, NamedTuple, TypeVar, Any
 )
+if sys.version_info[:2] >= (3, 7):  # pragma: no cover
+    from typing import Self


AFAIK, Self will be available on Python 3.11

another solution, following that approach could be

if sys.version_info[:2] >= (3, 7): # pragma: no cover from __future__ import annotations else: Result = TypeVar('Result', bound='Result') ... ... @staticmethod def load(filename: str) -> Result

unfortunately not as you can not redefine Result

Also

SyntaxError: from __future__ imports must occur at the beginning of the file

seems like no if clause is allowed to do this conditionally

and yes Self exists with python 3.11 not earlier... so that try also did not work

I hate to say that, but after several testing of ideas shared here I still think the less intrusive change to keep type annotations working with python 3.6 still supported is the proposed "weaken" change

I agree SLE15 provides alternative interpreters but plain Leap15 without backports doesn't and I think 3.6 might still be in use elsewhere.

Thoughts ?

I'm fine with weakening...

With the original code, adding # type: ignore[type-var] to the method signature works, no errors in mypy no errors in flake8

def load(filename: str) -> result_type: # type: ignore[type-var]

> mypy kiwi/system/result.py Success: no issues found in 1 source file > flake8 --statistics -j auto --count kiwi/system/result.py 0

If decided to go with the weakening for that method/type, that is fine by me as well

With the original code, adding # type: ignore[type-var] to the method signature works, no errors in mypy no errors in flake8

yes this seems to be the most efficient way to ignore it for the moment. Thanks much @jesusbv

@jesusbv

I adapted the code to use your ignore statement, mypy also seems to handle it but it still fails with an exit code != 0 which means mypy overall result status is failed, as you can see from the test runs here. This crap starts to bother
me :)

How did you make it to succeed ?

unit_py3_8 run-test: commands[1] | bash -c 'cd ../../ && mypy kiwi' kiwi/system/result.py:165: note: Consider using the upper bound "Result" instead kiwi/system/result.py:165: note: Error code "misc" not covered by "type: ignore" comment Success: no issues found in 188 source files ERROR: InvocationError for command /usr/bin/bash -c 'cd ../../ && mypy kiwi' (exited with code 1)

Thanks

TL;DR
Solution is not to use

def load(filename: str) -> result_type: # type: ignore[type-var]

but

def load(filename: str) -> result_type: # type: ignore

I checked

> mypy --version mypy 0.981 (compiled: no) > mypy kiwi/system/result.py Success: no issues found in 1 source file > python3.8 -m mypy --version mypy 0.790 > python3.8 -m mypy kiwi/system/result.py Success: no issues found in 1 source file > tox -e unit_py3_8 ..... unit_py3_8 run-test: commands[1] | bash -c 'cd ../../ && mypy kiwi' kiwi/system/result.py:118: note: Consider using the upper bound "Result" instead kiwi/system/result.py:118: note: Error code "misc" not covered by "type: ignore" comment Success: no issues found in 183 source files ERROR: InvocationError for command /bin/bash -c 'cd ../../ && mypy kiwi' (exited with code 1) _________________________________________________________ summary __________________________________________________________ ERROR: unit_py3_8: commands failed > mypy kiwi Success: no issues found in 183 source files > bash -c 'mypy kiwi' Success: no issues found in 183 source files > cd test/unit/ > bash -c 'cd ../../ && mypy kiwi' Success: no issues found in 183 source files

From that output I understand mypy is OK but the way to call it inside kiwi or something else related to the way it is invoked is borked/has an issue

Taking a look at more verbose output, mypy version is 0.982 which also says no errors

> tox -rvve unit_py3_8 ... Collecting mypy Using cached mypy-0.982-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (17.4 MB) ... [notice] A new release of pip available: 22.2.2 -> 22.3 [notice] To update, run: pip install --upgrade pip unit_py3_8 finish: getenv path/to/kiwi/.tox/3.8 after 40.05 seconds .... Success: no issues found in 183 source files ERROR: InvocationError for command /bin/bash -c 'cd ../../ && mypy kiwi' (exited with code 1) unit_py3_8 finish: run-test after 3.09 seconds unit_py3_8 start: run-test-post unit_py3_8 finish: run-test-post after 0.00 seconds

So, I can confirm something is borked, just not in kiwi
errors shown when running tox with # type: ignore]type-var] however, if the code is

def load(filename: str) -> result_type: # type: ignore

outputs are:

> tox -e unit_py3_8 unit_py3_8 run-test: commands[1] | bash -c 'cd ../../ && mypy kiwi' Success: no issues found in 188 source files _______________________________________________ summary _______________________________________________ unit_py3_8: commands succeeded congratulations :) > tox -e unit_py3_10 Finished processing dependencies for kiwi==9.24.48 unit_py3_10 run-test: commands[1] | bash -c 'cd ../../ && mypy kiwi' Success: no issues found in 188 source files _______________________________________________ summary _______________________________________________ unit_py3_10: commands succeeded congratulations :) >

Not sure if there is much interest in debugging why it is happening so, I may debug a bit to see the difference between type: ignore[type-var] vs type: ignore but I understand we are OK with that comment to ignore the error

jesusbv

Thanks for the changes

LGTM

schaefi · 2022-10-21T09:11:39Z

Thanks for the changes

LGTM

Thanks to you for all the debugging and input, much appreciated. The final change is super simple now :)

With an update of mypy the bound TypeVar is no longer allowed. In newer versions of python we could use the "Self" type or import annotations from the future module. Unfortunately in older python versions which we still support (3.6) there is no non intrusive change which allows us to handle that type annotation. Thus this commit ignores the return type spec for Result.load() for the moment.

schaefi requested review from Conan-Kudo, jesusbv and davidcassany October 15, 2022 15:14

schaefi self-assigned this Oct 15, 2022

Conan-Kudo requested a review from dcermak October 15, 2022 15:39

schaefi mentioned this pull request Oct 15, 2022

Bind-mount /dev into the OCI container instead of rsync-ing it #2195

Merged

jesusbv reviewed Oct 17, 2022

View reviewed changes

schaefi changed the title ~~weaken the type check on the Result class~~ Use Self from typing for python >= 3.7 else stick with TypeVar Oct 18, 2022

jesusbv reviewed Oct 18, 2022

View reviewed changes

schaefi force-pushed the weaken_type_check branch from 0ac9d58 to ea95e6e Compare October 19, 2022 06:21

schaefi changed the title ~~Use Self from typing for python >= 3.7 else stick with TypeVar~~ Weaken type check in the Result class Oct 19, 2022

schaefi changed the title ~~Weaken type check in the Result class~~ Weaken type check on Result.load() return type Oct 19, 2022

jesusbv self-requested a review October 21, 2022 08:43

jesusbv approved these changes Oct 21, 2022

View reviewed changes

schaefi force-pushed the weaken_type_check branch from 57f8a67 to b7950d9 Compare October 21, 2022 09:34

schaefi merged commit f055861 into master Oct 21, 2022

schaefi deleted the weaken_type_check branch October 21, 2022 12:38

Weaken type check on Result.load() return type #2196

Weaken type check on Result.load() return type #2196

Uh oh!

Conversation

schaefi commented Oct 15, 2022

Uh oh!

jesusbv Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesusbv Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schaefi Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Conan-Kudo Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

jesusbv Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schaefi Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesusbv Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesusbv left a comment

Choose a reason for hiding this comment

Uh oh!

schaefi commented Oct 21, 2022

Uh oh!

Uh oh!

jesusbv Oct 17, 2022 •

edited

Loading

jesusbv Oct 17, 2022 •

edited

Loading

schaefi Oct 17, 2022 •

edited

Loading

Conan-Kudo Oct 17, 2022 •

edited

Loading

jesusbv Oct 19, 2022 •

edited

Loading

schaefi Oct 20, 2022 •

edited

Loading

jesusbv Oct 20, 2022 •

edited

Loading