Add mypy build to Azure pipelines

[rodrigc]

https://twistedmatrix.com/trac/ticket/9848

[adiroiban]

just a comment

Since the build is allowed to fail, and at the same time not directly visible in the "Show all checks" list, I don't know who will check it before a merge.

I guess that merging this will not hurt that much...but if this is not used, it will just create extra load for Travis-CI and will slow down (just a tiny bit) the whole Travis-CI systems for all the other open source projects.

If a CI check is to be added, I think that is better create a separate mypy-ci TOX environment which will run mypy with a specific configuration file in such a way that the build is green.

Later, while changes are made, the mypy-ci configuration file can be updated to include other parts of the Twisted code base

---

From my experience, if you have a build with "allow_fail" it means that the build has no importance and it will never be fixed and will just accumulate more failures.
Is better to add a test with some exceptions, and make sure is green and make sure that at least no regressions are added.

I think that this is the approach we use (and still use) for pyhon3 porting and it was used for the pyflakes cleanup.

Bellow is a suggestion of how this new mypy-ci could looks like.

Btw... It looks like the mypi.ini file is not used by tox... not sure why it is theres.

diff --git a/mypy.ini b/mypy.ini
index 9d4f38bc7..b1fce7f47 100644
--- a/mypy.ini
+++ b/mypy.ini
@@ -18,3 +18,67 @@ disallow_incomplete_defs = False
 no_implicit_optional     = False
 warn_return_any          = False
 warn_unreachable         = False
+
+
+[mypy-twisted.application.*]
+ignore_errors = True
+
+
+[mypy-twisted.logger.*]
+ignore_errors = True
+
+[mypy-twisted.pair.*]
+ignore_errors = True
+
+
+[mypy-twisted.protocols.*]
+ignore_errors = True
+
+
+[mypy-twisted.python.test.*]
+ignore_errors = True
+
+
+[mypy-twisted.test.*]
+ignore_errors = True
+
+
+[mypy-twisted.conch.test.*]
+ignore_errors = True
+
+
+[mypy-twisted.cred.test.*]
+ignore_errors = True
+
+
+[mypy-twisted.internet.test.*]
+ignore_errors = True
+
+[mypy-twisted.mail.*]
+ignore_errors = True
+
+[mypy-twisted.names.test.*]
+ignore_errors = True
+
+
+[mypy-twisted.trial.*]
+ignore_errors = True
+
+[mypy-twisted._threads.test.*]
+ignore_errors = True
+
+
+[mypy-twisted.trial._dist.test.*]
+ignore_errors = True
+
+
+[mypy-twisted.scripts.*]
+ignore_errors = True
+
+
+[mypy-twisted.web.test.*]
+ignore_errors = True
+
+
+[mypy-twisted.words.*]
+ignore_errors = True
diff --git a/tox.ini b/tox.ini
index c391de715..b272d9de6 100644
--- a/tox.ini
+++ b/tox.ini
@@ -172,5 +172,6 @@ deps =
 commands =
     mypy                                       \
         --cache-dir="{toxworkdir}/mypy_cache"  \
+        --config="{toxinidir}/mypy.ini"        \
         {tty:--pretty:}                        \
         {posargs:src}

[rodrigc]

The change to tox.ini that you suggested is not needed.
The mypy.ini is used. By default if you do not specify the --config flag, mypy will look for mypy.ini the directory where it was invoked.

If I hack mypy.ini to introduce errors, and then do: tox -e mypy, I see:

mypy.ini: File contains no section headers.
file: 'mypy.ini', line: 1
'mypy]\n'

[rodrigc]

In terms of resources, on my laptop, if I run:
tox -e mypy , it takes:

tox -e mypy  31.04s user 7.22s system 97% cpu 39.223 total

So the resources are not too bad.

[rodrigc]

To address your concern about not being able to see the mypy build, I removed it from Travis,
and added it to Azure pipelines.
The Azure pipeline is configured to always succeed, so it won't block the build. From the GitHub PR, you can see a link for twisted.twisted (Mypy Ubuntu) which takes you directly to the mypy results.

When we eliminate all mypy errors, we can reconfigure the Azure pipeline to fail if mypy fails.

Last week there were over 1200 errors reported by mypy, and right now there are around 550, so I think it is doable over time.

[adiroiban]

I would prefer to have the jobs defined in a single file and see the actual result of the tests.

Other than that, all fine.

You are right, there is no need to explicitly call mypy with the path to the configuration file.

[adiroiban]

Thanks for the change.

I think that this is a bit of an over-engineering.

For mypy the "template" should be use only as an include semantic and not as a generator.

I feel that splitting the rules over 2 files, and defining the Ubuntu image name as a variable is only making it harder to understand what is going on there.

Below is my suggestions for the content of this file, and the other file can be removed.

Also, since this is only one job, I think that is best to name this file just mypy.job.yml or something like that.

This is all about testing so using test in the name doesn't add much value... and this is a single job ...so jobs is kind of misleading.

I would prefer to see the test in red, as failing... so that we can see the actual result.

It should not be marked as required, so it should not block any merge.

But it will serve as a reminder that mypy is not really green.

jobs:
- job: 'ubuntu_mypy'
  displayName: 'Mypy Ubuntu'
  pool:
    vmImage: 'ubuntu-latest'
  steps:
  - task: UsePythonVersion@0
    displayName: "Use Python 3.8"
    inputs:
      versionSpec: "3.8"

  - script: |
     python --version
     python -c "import sys; print(sys.prefix)"
     python -c "import sys; print(sys.exec_prefix)"
     python -c "import sys; print(sys.executable)"
     python -c "import struct; print(struct.calcsize('P') * 8)"
    displayName: 'Get Python Information'

  - script: 'python -m pip install -U pip setuptools tox'
    displayName: 'Update pip & install tox'

  - script: 'python -m tox -e mypy'
    displayName: 'Run mypy'

[rodrigc]

OK, I changed this PR to have a single mypy_jobs.yml file. With the latest changes, the Mypy job ran here: https://github.com/twisted/twisted/pull/1290/checks?check_run_id=781768030

[adiroiban]

Looks good. Thanks.

The status is green, when in fact the build failed.

I think is best not to fake it, and instead show the real status.

I don't see what we gain if we fake the status.

[rodrigc]

OK, I can take out the setting continueOnError: true

Since the Mypy build is not configured as a build that is required to pass in order to merge a PR,
this is OK. Once we get down to zero mypy errors, we can toggle this build as a mandatory build.

[rodrigc]

@adiroiban So I removed continueOnError: true from the Azure mypy build, sot it fails, but now I cannot merge, becaue it looks like all Azure builds are required to succeed:

Should I put continueOnError: true back into the mypy job config for the Azure pipeline, or is there some other setting that can be done on the GitHub side, so that even if the status for the mypy build is failed, we can still merge?

[rodrigc]

@adiroiban by the way, the Windows Python 3.8 builds on Azure have been marked as continueOnError: true, because due to this: https://twistedmatrix.com/trac/ticket/9766

You can see the Windows Python 3.8 builds fail here:
https://dev.azure.com/twistedmatrix/twisted/_build/results?buildId=1918&view=logs&j=ea01aad7-d5f3-5f0e-2fda-71d72e596cac&t=f47a629b-ce24-5ee0-314a-24c887b72299&l=15944

Unless there is a way to flag certain Azure builds as required,
right now all the Azure builds need to pass, otherwise the PR is unmergeable.

Do you know if we can change the required builds?

[adiroiban]

true... as mypy is part of the twisted.twisted pipeline

I think that the mypy should renamed to mypy_pipeline.yml and removed from tests_pipeline.yml

This will be a simple pipeline definition with a single stage/job

displayName: 'Mypy Ubuntu'
pool:
  vmImage: 'ubuntu-latest'
steps:
- task: UsePythonVersion@0
  displayName: "Use Python 3.8"
  inputs:
    versionSpec: "3.8"

- script: |
   python --version
   python -c "import sys; print(sys.prefix)"
   python -c "import sys; print(sys.exec_prefix)"
   python -c "import sys; print(sys.executable)"
   python -c "import struct; print(struct.calcsize('P') * 8)"
  displayName: 'Get Python Information'
- script: 'python -m pip install -U pip setuptools tox'
  displayName: 'Update pip & install tox'

- script: 'python -m tox -e mypy'
  displayName: 'Run mypy'

I don't have access to Azure Devops...so I can't create a new pipeline.
So I guess that for now we can merge it with ignoring the errors unless @hawkowl or @glyph can help with setting up a new pipeline.

Thanks!

[rodrigc]

Sounds good. Thanks for your patience and review feedback!

[adiroiban]

I have access to twisted/twisted protected branch settings and I can change which test is required... but I prefer to have mypy as a separate pipeline... otherwise I will have to disable the twisted.twisted main pipeline requirements and require separate jobs....

But maybe this is the best way forward as it will only require GitHub access without extra access to Azure Devops (for creating an extra pipeline)