Bootstraps type annotations #1069

happz · 2022-02-24T11:57:38Z

The patch contains basic infrastructure and annotates one hand-picked
file, tmt/utils.py.

Feel free to ignore

I just wanted to share the work, and save it in the repo for later. Adding annotations revealed a couple of patch-worthy issues, and my plan is to fix them in separate patches before dropping this patch with annotation on top of it. Therefore draft at this moment, and it will get more changes and rebases.

FrNecas · 2022-02-24T12:30:38Z

Nice, really happy that we are slowly adding type annotations. Let me know if I can help you somehow with annotating. I agreed with @psss that I will try taking a look at the paralelization of execute step for multihost so this is basically the first step. I was planning on annotating some of the modules, possibly refactoring some things on the way

psss · 2022-02-28T11:41:47Z

Great, thanks for starting work on this!

happz · 2022-03-01T13:32:26Z

Nice, really happy that we are slowly adding type annotations. Let me know if I can help you somehow with annotating. I agreed with @psss that I will try taking a look at the paralelization of execute step for multihost so this is basically the first step. I was planning on annotating some of the modules, possibly refactoring some things on the way

Sure, I plan to ask a lot of questions while working on this, that would be extremely helpful :)

My general plan is to bootstrap the annotations by adding the necessary infrastructure plus annotating a module, to test things do work as expected, then I would look into annotating a core module plus one of the steps plugins, to learn more about the internals, and after that, I'd consider things to be stable enough that anyone should be able to work on the rest of modules in pretty much any given order. Right now, in the beginning, I'd say the gradual approach is the safest bet, but once things settle and we'd learn how steps interact with core on this level, multiple people can work on different modules without running into conflicts.

happz · 2022-03-09T09:56:23Z

Rebased & updated. The last remaining piece of code not annotated is StructuredField - here I wait for #1085 to land, then I'd annotate this class. Annotating first and then removing basestring and unicode would lead to nasty rebase with conflicts.

happz · 2022-03-14T17:01:27Z

Rebased & updated, and I get zero errors locally. Open for review.

What's missing? Actually running the test: I have a piece in .pre-commit-config.yaml, commented out, but I'd like to get guidance on how to enable type annotation checking. It's on par with stuff like PEP8, hence my proposal for pre-commit, but if there's more to add, let me know.

And there are few unit tests failing, I suppose those are related to my changes, breaking rpm builds.

thrix · 2022-03-14T17:06:44Z

interesting, some unit tests fail during koji build

thrix · 2022-03-14T17:07:44Z

@happz pre-commit makes sense, it already runs via github actions anyway

FrNecas

The typing overall looks good to me, thanks for this work! Just some questions/requests for clarifications from me, otherwise not much else to add. I will try to help with typing more stuff once we merge this PR

mypy.ini

tmt/base.py

tmt/utils.py

happz · 2022-03-23T17:55:53Z

/packit test

tmt/utils.py

happz · 2022-03-23T18:54:09Z

I've spotted a couple of tests I probably cannot fix on my own:

plenty of Centos Stream 8 tests fail because of trouble with podman, podman: symbol lookup error: podman: undefined symbol: seccomp_notify_fd at https://artifacts.dev.testing-farm.io/0d703719-de06-4b6e-93a4-0d235d86cd23/
/tests/integration/coverage_bugzilla and /tests/integration/the_rest at https://artifacts.dev.testing-farm.io/0d703719-de06-4b6e-93a4-0d235d86cd23/ - these seem to fail because of failed imports, and these failures I'd expect to manifest in other PRs as well.

/usr/local/lib/python3.6/site-packages/git/types.py:12: in <module>
    from typing_extensions import Final, Literal, SupportsIndex  # noqa: F401
E   ImportError: cannot import name 'SupportsIndex'

Might be related to the fact typing_extensions dropped support for Python 3.6 a few days ago, or the system package is too old and does not provide SupportsIndex, but I don't see a convincing answer to why is this happening. Any ideas? Could it be caused by me messing with packages and types?

lukaszachy · 2022-03-24T15:08:42Z

Might be related to the fact typing_extensions dropped support for Python 3.6 a few days ago, or the system package is too old and does not provide SupportsIndex, but I don't see a convincing answer to why is this happening. Any ideas? Could it be caused by me messing with packages and types?

Looks as too old python3-typing-extensions. Can be reproduced if one installs dnf install python3-typing-extensions-3.7.4.2-1.el8.noarch and then pip-3 install requre. Import of require than fails.

And pip-3 install --upgrade typing-extensions upgrades to 4.1.1. After that import requre works

psss · 2022-03-29T07:34:08Z

plenty of Centos Stream 8 tests fail because of trouble with podman

This seems to be caused by a podman bug, the fix should be in production soon.

happz · 2022-03-29T07:40:11Z

Might be related to the fact typing_extensions dropped support for Python 3.6 a few days ago, or the system package is too old and does not provide SupportsIndex, but I don't see a convincing answer to why is this happening. Any ideas? Could it be caused by me messing with packages and types?

Looks as too old python3-typing-extensions. Can be reproduced if one installs dnf install python3-typing-extensions-3.7.4.2-1.el8.noarch and then pip-3 install requre. Import of require than fails.

And pip-3 install --upgrade typing-extensions upgrades to 4.1.1. After that import requre works

@lukaszachy shall I add this somewhere into my patch?

lukaszachy · 2022-03-29T08:15:17Z

@happz IMO you could add dependency on typing-extensions>=3.7.4.3

I've created epel-8 bug to have it updated https://bugzilla.redhat.com/show_bug.cgi?id=2069579

happz · 2022-03-29T08:26:41Z

@happz IMO you could add dependency on typing-extensions>=3.7.4.3

I've created epel-8 bug to have it updated https://bugzilla.redhat.com/show_bug.cgi?id=2069579

I'm afraid I'll need more help. Should I add it? If so, where? Is setup.py the right place? I think it is, but maybe changing tests' requirements would be better.

lukaszachy · 2022-03-29T08:30:32Z

IMO at two places:

plans/integration/main.fmf where script: pip3 install requre happens
setup.py to 'tests': ['pytest', 'python-coveralls', 'requre', 'pre-commit'],

We don't have requirements for pip packages (yet)

happz · 2022-03-29T11:13:43Z

IMO at two places:

plans/integration/main.fmf where script: pip3 install requre happens

setup.py to 'tests': ['pytest', 'python-coveralls', 'requre', 'pre-commit'],

Done both, didn't seem to have any effect.

I tried to reproduce locally, with the help of tmt reproducer provided in TF logs, and error happens a bit sooner:

$ tmt -vvv -c trigger=commit -c arch=x86_64 -c distro=centos-stream-8 run --all --verbose -e @tmt-environment-plans-integration.yaml provision --how virtual --image CentOS-Stream-8 plan --name /plans/integration
/var/tmp/tmt/run-005
Found 1 plan.

/plans/integration
summary: Verify integration with other systems
    discover
        how: fmf
        order: 50
        directory: /tmp/testcode
        hash: 96828a0
        filter: tag: integration
        summary: 2 tests selected
            /tests/integration/coverage_bugzilla
            /tests/integration/the_rest
    provision
        how: virtual
        order: 50
        image: CentOS-Stream-8
        user: root
        memory: 2048 MB
        disk: 10 GB
        connection: session
        qcow: CentOS-Stream-GenericCloud-8-20210603.0.x86_64.qcow2
        name: tmt-005-vPKxxice
        progress: booting...
        ip: 127.0.0.1
        port: 10023
        distro: CentOS Stream 8
        kernel: 4.18.0-305.3.1.el8.x86_64
        summary: 1 guest provisioned
    prepare
        how: install
        name: Install pip
        order: 50
        package: 1 package requested
            python3-pip
    
        how: shell
        name: Install Requre
        order: 50
        overview: 1 script found
        script: pip3 install typing-extensions>=3.7.4.3 requre
    
        how: shell
        name: Create default nitrate config
        order: 50
        overview: 1 script found
        script: 
            test -e ~/.nitrate || echo -en '[nitrate]
            url = https://nitrate.server/xmlrpc/
            ' | tee ~/.nitrate
    
        how: install
        summary: Install required packages
        name: requires
        order: 70
        package: 1 package requested
            python3-pytest
    
        summary: 4 preparations applied
    execute
        how: tmt
        order: 50
        exit-first: False
            test: Integration tests with requre support
                out: ============================= test session starts ==============================
                out: platform linux -- Python 3.6.8, pytest-7.0.1, pluggy-1.0.0 -- /usr/bin/python3
                out: cachedir: .pytest_cache
                out: rootdir: /var/tmp/tmt/run-005/plans/integration/discover/default/tests
                out: collecting ... collected 0 items / 1 error
                out: 
                out: ==================================== ERRORS ====================================
                out: ______________ ERROR collecting tests/integration/test_nitrate.py ______________
                out: ImportError while importing test module '/var/tmp/tmt/run-005/plans/integration/discover/default/tests/tests/integration/test_nitrate.py'.
                out: Hint: make sure your test modules/packages have valid Python names.
                out: Traceback:
                out: /usr/lib64/python3.6/importlib/__init__.py:126: in import_module
                out:     return _bootstrap._gcd_import(name[level:], package, level)
                out: __init__.py:3: in <module>
                out:     import nitrate
                out: E   ModuleNotFoundError: No module named 'nitrate'
                out: =========================== short test summary info ============================
                out: ERROR test_nitrate.py
                out: !!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
                out: =============================== 1 error in 0.09s ===============================
                00:00:00 /tests/integration/coverage_bugzilla [1/2]
            test: Integration tests with requre support
                out: ============================= test session starts ==============================
                out: platform linux -- Python 3.6.8, pytest-7.0.1, pluggy-1.0.0 -- /usr/bin/python3
                out: cachedir: .pytest_cache
                out: rootdir: /var/tmp/tmt/run-005/plans/integration/discover/default/tests
                out: collecting ... collected 0 items / 1 error
                out: 
                out: ==================================== ERRORS ====================================
                out: ______________ ERROR collecting tests/integration/test_nitrate.py ______________
                out: ImportError while importing test module '/var/tmp/tmt/run-005/plans/integration/discover/default/tests/tests/integration/test_nitrate.py'.
                out: Hint: make sure your test modules/packages have valid Python names.
                out: Traceback:
                out: /usr/lib64/python3.6/importlib/__init__.py:126: in import_module
                out:     return _bootstrap._gcd_import(name[level:], package, level)
                out: __init__.py:3: in <module>
                out:     import nitrate
                out: E   ModuleNotFoundError: No module named 'nitrate'
                out: =========================== short test summary info ============================
                out: ERROR test_nitrate.py
                out: !!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
                out: =============================== 1 error in 0.09s ===============================
                00:00:00 /tests/integration/the_rest [2/2]
        summary: 2 tests executed
    report
        how: display
        order: 50
            errr /tests/integration/coverage_bugzilla
                output.txt: /var/tmp/tmt/run-005/plans/integration/execute/data/tests/integration/coverage_bugzilla/output.txt
                content: 
                    ============================= test session starts ==============================
                    platform linux -- Python 3.6.8, pytest-7.0.1, pluggy-1.0.0 -- /usr/bin/python3
                    cachedir: .pytest_cache
                    rootdir: /var/tmp/tmt/run-005/plans/integration/discover/default/tests
                    collecting ... collected 0 items / 1 error
                    
                    ==================================== ERRORS ====================================
                    ______________ ERROR collecting tests/integration/test_nitrate.py ______________
                    ImportError while importing test module '/var/tmp/tmt/run-005/plans/integration/discover/default/tests/tests/integration/test_nitrate.py'.
                    Hint: make sure your test modules/packages have valid Python names.
                    Traceback:
                    /usr/lib64/python3.6/importlib/__init__.py:126: in import_module
                        return _bootstrap._gcd_import(name[level:], package, level)
                    __init__.py:3: in <module>
                        import nitrate
                    E   ModuleNotFoundError: No module named 'nitrate'
                    =========================== short test summary info ============================
                    ERROR test_nitrate.py
                    !!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
                    =============================== 1 error in 0.09s ===============================
            errr /tests/integration/the_rest
                output.txt: /var/tmp/tmt/run-005/plans/integration/execute/data/tests/integration/the_rest/output.txt
                content: 
                    ============================= test session starts ==============================
                    platform linux -- Python 3.6.8, pytest-7.0.1, pluggy-1.0.0 -- /usr/bin/python3
                    cachedir: .pytest_cache
                    rootdir: /var/tmp/tmt/run-005/plans/integration/discover/default/tests
                    collecting ... collected 0 items / 1 error
                    
                    ==================================== ERRORS ====================================
                    ______________ ERROR collecting tests/integration/test_nitrate.py ______________
                    ImportError while importing test module '/var/tmp/tmt/run-005/plans/integration/discover/default/tests/tests/integration/test_nitrate.py'.
                    Hint: make sure your test modules/packages have valid Python names.
                    Traceback:
                    /usr/lib64/python3.6/importlib/__init__.py:126: in import_module
                        return _bootstrap._gcd_import(name[level:], package, level)
                    __init__.py:3: in <module>
                        import nitrate
                    E   ModuleNotFoundError: No module named 'nitrate'
                    =========================== short test summary info ============================
                    ERROR test_nitrate.py
                    !!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
                    =============================== 1 error in 0.09s ===============================
        summary: 2 errors
    finish
        guest: stopped
        guest: removed
        summary: 0 tasks completed

total: 2 errors

Why is nitrate missing? I couldn't find it anywhere in test requirements, it's in setup.py for convert extra, but that's not a problem in the context of a test when executed by Testing Farm, triggers when running it locally. A test bug?

Another point I don't understand is why is this happening when I got anywhere close to messing with nitrate or its tests? I'm not aware of changes affecting this area of code, is this really the only patch having troubles with tests on CentOS? And if so, what changes are causing those troubles?

plans/integration/main.fmf

tmt/utils.py

thrix · 2022-04-14T10:53:49Z

/packit test

psss · 2022-04-25T13:36:04Z

Two tests were failing because yaml_to_dict() is now more strict about the yaml input. Should be fixed by 960eaca. Also rebased on the latest main content.

TomasTomecek · 2022-04-26T09:27:48Z

/packit build

psss

Thanks a lot for kicking off the type annotations! It must have been a real patience exercise. At least it was for me ;-) And sorry for taking so long with the review. Overall looks good to me. Added just a couple of comments and committed a few minor adjustments in 68d9841.

tests/integration/main.fmf

tmt/steps/__init__.py

tmt/utils.py

psss · 2022-05-09T18:47:38Z

Squashed and rebased. A couple of adjustments are needed for updatable_message:

tmt/utils.py:2014: error: Missing type parameters for generic type "contextlib.AbstractContextManager"
tmt/utils.py:2061: error: Function is missing a type annotation
tmt/utils.py:2065: error: Function is missing a type annotation for one or more arguments
Found 3 errors in 1 file (checked 1 source file)

Tried to fix the errors in e1ad5c6. @happz, please have a look.

happz · 2022-05-09T19:12:41Z

Squashed and rebased. A couple of adjustments are needed for updatable_message:
tmt/utils.py:2014: error: Missing type parameters for generic type "contextlib.AbstractContextManager"
tmt/utils.py:2061: error: Function is missing a type annotation
tmt/utils.py:2065: error: Function is missing a type annotation for one or more arguments
Found 3 errors in 1 file (checked 1 source file)
Tried to fix the errors in e1ad5c6. @happz, please have a look.

All look good to me with the exception of the AbstractContextManager one. I'm afraid there might be troubles in runtime, did you try importing the fixed file or running tmt? If I get this right, with some Python versions this may end up with an error, something along the lines of "ABCMeta object is no subscribable", because AbstractContextManager might indeed be nothing one could apply an index to.

I've found a couple of articles discussing how to annotate context managers correctly, e.g. https://peps.python.org/pep-0585/#implementation, my impression is the answer depends heavily on which Python version one needs to address. If tmt works for you with this patch, then good, surprising, but good - but if not, this seems to be a can of worms, and I'd propose adding # type: ignore to silence mypy, and I could address it in a separate patch.

psss · 2022-05-09T19:17:35Z

Seems to be working fine with recent Pythons on Fedora but fails on el8 and el9.

I'd propose adding # type: ignore to silence mypy, and I could address it in a separate patch.

Ok, will add this.

psss · 2022-05-10T07:16:27Z

Tests passed, for el8 there's the usual pile of podman failures but el9 build failed:

No matching package to install: 'python3-typing-extensions'

There is already a bug asking to add the package to epel9. Seems to be blocked by some other missing dependencies. We could possibly use typing directly on el9 as the python version is quite fresh there: Python 3.9.10 But that would require some conditional imports I guess.

@happz, what do you think? Shall we wait for python3-typing-extensions or try to workaround it? If I understand it correctly el8 is the only distro where we need extensions because of the old Python 3.6.8 version

happz · 2022-05-10T08:17:14Z

Tests passed, for el8 there's the usual pile of podman failures but el9 build failed:
No matching package to install: 'python3-typing-extensions'
There is already a bug asking to add the package to epel9. Seems to be blocked by some other missing dependencies. We could possibly use typing directly on el9 as the python version is quite fresh there: Python 3.9.10 But that would require some conditional imports I guess.

@happz, what do you think? Shall we wait for python3-typing-extensions or try to workaround it? If I understand it correctly el8 is the only distro where we need extensions because of the old Python 3.6.8 version

Trying a conditional import and BuildRequires in b878321, let's see how it turns out.

psss · 2022-05-10T15:23:06Z

Works nicely! Only the spec file condition requires typing-extensions for Fedora as well. Will fix that.

The patch contains basic infrastructure and annotates one hand-picked file, `tmt/utils.py`. Co-authored-by: Petr Šplíchal <psplicha@redhat.com>

psss

Everything looks good now! @happz, thanks a lot for kicking this off!

happz · 2022-05-11T08:03:46Z

Cool, thanks for the review & final merging.

And here we go... #1170 :)

psss marked this pull request as draft February 28, 2022 11:39

happz force-pushed the mypy-bootstrap branch from 98d8bdb to 5b40544 Compare March 9, 2022 09:54

happz force-pushed the mypy-bootstrap branch from 5b40544 to 616bf61 Compare March 14, 2022 16:57

happz force-pushed the mypy-bootstrap branch from 616bf61 to fb046e2 Compare March 14, 2022 21:40

happz changed the title ~~Draft: Bootstraps type annotations~~ Bootstraps type annotations Mar 14, 2022

happz marked this pull request as ready for review March 14, 2022 21:41

happz force-pushed the mypy-bootstrap branch from fb046e2 to 923b788 Compare March 14, 2022 21:49

happz force-pushed the mypy-bootstrap branch from a63487c to d1a3aed Compare March 22, 2022 18:25

FrNecas reviewed Mar 22, 2022

View reviewed changes

mypy.ini Show resolved Hide resolved

tmt/base.py Show resolved Hide resolved

tmt/utils.py Outdated Show resolved Hide resolved

tmt/utils.py Outdated Show resolved Hide resolved

tmt/utils.py Outdated Show resolved Hide resolved

tmt/utils.py Outdated Show resolved Hide resolved

psss reviewed Mar 23, 2022

View reviewed changes

tmt/utils.py Outdated Show resolved Hide resolved

FrNecas reviewed Mar 29, 2022

View reviewed changes

plans/integration/main.fmf Show resolved Hide resolved

psss force-pushed the mypy-bootstrap branch from 2839eb2 to c7da3ae Compare March 30, 2022 15:57

psss reviewed Mar 30, 2022

View reviewed changes

tmt/utils.py Outdated Show resolved Hide resolved

psss added this to the 1.13 milestone Mar 31, 2022

psss force-pushed the mypy-bootstrap branch from 8b816ab to 960eaca Compare April 25, 2022 13:33

psss self-assigned this Apr 25, 2022

psss added the style label Apr 25, 2022

psss modified the milestones: 1.13, 1.14 May 2, 2022

psss requested changes May 6, 2022

View reviewed changes

psss reviewed May 9, 2022

View reviewed changes

tmt/utils.py Show resolved Hide resolved

psss force-pushed the mypy-bootstrap branch from 4113285 to e1ad5c6 Compare May 9, 2022 18:43

psss force-pushed the mypy-bootstrap branch from e1ad5c6 to eefc619 Compare May 9, 2022 19:18

Bootstrap type annotations

2d297fc

The patch contains basic infrastructure and annotates one hand-picked file, `tmt/utils.py`. Co-authored-by: Petr Šplíchal <psplicha@redhat.com>

psss force-pushed the mypy-bootstrap branch from 4eea771 to 2d297fc Compare May 10, 2022 15:28

psss approved these changes May 11, 2022

View reviewed changes

psss merged commit 2d297fc into teemtee:main May 11, 2022

psss added type annotations Related to type annotations and type cleanup and removed style labels Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bootstraps type annotations #1069

Bootstraps type annotations #1069

happz commented Feb 24, 2022 •

edited

FrNecas commented Feb 24, 2022

psss commented Feb 28, 2022

happz commented Mar 1, 2022

happz commented Mar 9, 2022

happz commented Mar 14, 2022 •

edited

thrix commented Mar 14, 2022

thrix commented Mar 14, 2022

FrNecas left a comment

happz commented Mar 23, 2022

happz commented Mar 23, 2022

lukaszachy commented Mar 24, 2022

psss commented Mar 29, 2022

happz commented Mar 29, 2022

lukaszachy commented Mar 29, 2022 •

edited

happz commented Mar 29, 2022

lukaszachy commented Mar 29, 2022

happz commented Mar 29, 2022

thrix commented Apr 14, 2022

psss commented Apr 25, 2022

TomasTomecek commented Apr 26, 2022

psss left a comment

psss commented May 9, 2022

happz commented May 9, 2022

psss commented May 9, 2022

psss commented May 10, 2022

happz commented May 10, 2022 •

edited

psss commented May 10, 2022

psss left a comment

happz commented May 11, 2022

Bootstraps type annotations #1069

Bootstraps type annotations #1069

Conversation

happz commented Feb 24, 2022 • edited

Feel free to ignore

FrNecas commented Feb 24, 2022

psss commented Feb 28, 2022

happz commented Mar 1, 2022

happz commented Mar 9, 2022

happz commented Mar 14, 2022 • edited

thrix commented Mar 14, 2022

thrix commented Mar 14, 2022

FrNecas left a comment

Choose a reason for hiding this comment

happz commented Mar 23, 2022

happz commented Mar 23, 2022

lukaszachy commented Mar 24, 2022

psss commented Mar 29, 2022

happz commented Mar 29, 2022

lukaszachy commented Mar 29, 2022 • edited

happz commented Mar 29, 2022

lukaszachy commented Mar 29, 2022

happz commented Mar 29, 2022

thrix commented Apr 14, 2022

psss commented Apr 25, 2022

TomasTomecek commented Apr 26, 2022

psss left a comment

Choose a reason for hiding this comment

psss commented May 9, 2022

happz commented May 9, 2022

psss commented May 9, 2022

psss commented May 10, 2022

happz commented May 10, 2022 • edited

psss commented May 10, 2022

psss left a comment

Choose a reason for hiding this comment

happz commented May 11, 2022

happz commented Feb 24, 2022 •

edited

happz commented Mar 14, 2022 •

edited

lukaszachy commented Mar 29, 2022 •

edited

happz commented May 10, 2022 •

edited