Start using pytype (and mypy) to ensure broken Python3 code has a bad time in the future. #5365

bernhardkaindl · 2024-01-17T13:48:13Z

This PR is just a "demonstrator". It just shows a bit of what can be done for Python CI.

It is not meant to be merged in this form. Rather, the plan is to take the bits into small PRs.
By showing how which kind of changes we'd need to make to ensure correct migration to Python3, I think it also shows that quite some code testing might be needed.

Context:

I've seen that Edwin, Pau and Rob had to revert the broken (and vastly incomplete) #5296 with #5361

In the description of #5361, it was shown that the error was:

os.write(fd, config)\x0A\x0ATypeError: a bytes-like object is required, not 'str'\x0A\x0A ] ]

Immediately when I saw this, I checked if mypy may have found it in GitHub CI with the mypy config I have for the other Python3 migrations that I've done for the XenServer organisation here on GitHub over the course of the last 12 months during which I gained a lot of experience.

Result: It would have warned about a different Python3 migration issue where it writes b"some-string" because of bytes/string mismatch in Python3:

scripts/mail-alarm: note: In function "main":
scripts/mail-alarm:1018: error: If x = b'abc' then "%s" % x produces "b'abc'", not "abc".
If this is desired behavior use "%r" % x. Otherwise, decode the bytes  [str-bytes-safe]
                sender = "noreply@%s" % getfqdn().encode(charset)

When I saw it, I cried out loud! 🥵

Then, I fired off pytype I that I had used extensively for the Python3 migration of python-libs !

AND... It actually finds the bug:

scripts/mail-alarm", line 1027, in main: Function os.write was called with the wrong arguments [wrong-arg-types]
         Expected: (__fd, __data: Buffer)
  Actually passed: (__fd, __data: str)

And there is (at least...) a 3rd bug that mypy and pytype find: Python3 has no popen2 function:

pytype:
scripts/mail-alarm", line 1031, in main: No attribute 'popen2' on module 'os' [module-attr]
mypy:
scripts/mail-alarm:1031: error: Module has no attribute "popen2"; maybe "popen"?  [attr-defined]
                chld_stdin, chld_stdout = os.popen2(
                                          ^~~~~~~~~

So, using mypy and pytype in CI will improve the quality of or PRs (even before we've to review them) a lot!

Open TODOs:

Re-base to the new master where scripts/mail-alarm and scripts/usb_scan.py have been restored to work.
When possible, add a check to pre-commit which ensures that files which are touched pass mypy and pyright.
When possible (maybe later) add also as check that they pass a set of pylint sanity checks too.
Cleanup the to also pass the Python2 CI again.
Split it up so it mostly adds the configuration to leave large changes in production code for later PR after this is merged.

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>

…et pytype finish Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>

edwintorok

Thanks, some small comments.

edwintorok · 2024-01-17T14:15:56Z

.github/workflows/main.yml

+          --cov-report xml:.git/coverage27.xml
+
+      - name: Upload coverage reports to Codecov
+        uses: codecov/codecov-action@v3


Will this need something to be enabled on this repo for codecov to work? IIRC the app needs to be added to the repo, or has that already be done on an org level?

Yes, it will have to be added to the organisation or the repo by an admin in order to be able to make comments.

I can also/alternatively add coverage with coveralls.io, which might be easer to add, maybe.

The workflow also attempts to add a comment to the PR locally on Git if it has permission.

It would be good if I could be added as a member to the https://github.com/xapi-project or the repo at least.

I should be in Cambridge next week, so we'll see, hopefully!

if this needs additional permissions on the repo for a 3rdparty app maybe comment out this part of the PR for now, and we can come back to it later.

edwintorok · 2024-01-17T14:17:50Z

.github/workflows/main.yml

+            https://github.com/marketplace/actions/pytest-coverage-comment

-      - name: Run python tests
-        run: pytest scripts


I see this got moved to the precommit script, just adding a comment here for other reviewers.

edwintorok · 2024-01-17T14:20:50Z

run_pytype.py

@@ -0,0 +1,179 @@
+#!/usr/bin/env python


This is quite a lot of code, is this shared with xsconsole and other python projects? Would be good to add a comment where the "canonical" source for this lives (in case we improve it/fix it)

Yes, it comes from https://github.com/xenserver/python-libs/blob/master/run-pytype.py

Yes, we should turn it into a shared pre-commit hook repository (pre-commit will fetch it then when installing its hooks - will do so locally when we run pre-commit and in CI too!).

Then, we can configure it also in .pre-commit-config.yaml and in every python project for CI as well.

We've used this repository in the past to store project-wide CI-related settings (e.g. version numbers) https://github.com/xapi-project/xs-opam/blob/master/tools/xs-opam-ci.env
Not sure if thats the best place to store the python CI related settings, but it is one possibility

Probably best to coordinate with @psafont on how to best share/set up a python CI, he knows a lot more about Python than I do.

Maybe code sharing could be done as a custom Github action? Note there is an existing one (did not compare the 2 approaches, though)

Suggest to move this file in a separated repo for pre-commit, just like https://github.com/hoefling/pre-commit-pytype/tree/master.

So that we can reuse this hook without duplicating it.

Maybe code sharing could be done as a custom Github action? Note there is an existing one (did not compare the 2 approaches, though)

That one does not work it and it hasn't been updated in two years.

An additional problem with it is that it designed to run only as a Github acation, and has no configuration to run it from the command line, locally.

While it is possible to run a simple limited GH workflow locally inside Docker/Podman using act, it's not what I prefer to use because of its limitations and every developer who wants to test it needs to commit and push for each test, or setup act and Docker.

But the not working and not being updated side is the actual problem.

The pytyoe runner I created can produce very nice output on GitHub in the Overview page of the Action and is configurable in more ways than is possible with pytype.cfg alone, in pyprojectl.toml.

As it also runs locally and can be even used in a pre-push and pre-commit hook, mine leads to a much smoother workflow.

I see mine therefore as vastly superior.

Integration as a shared pre-commit hook like @aceifei suggested with an other example for pytype is exactly what I have in mind so to have only the config locally once the basics are sorted out, as plan.

I see mine therefore as vastly superior.

Can it be packaged so it's easily installed using pip / pipx ? This would allow for code sharing as well as make it easily usable in any environment

edwintorok · 2024-01-17T14:22:01Z

scripts/yum/plugins.py

@@ -0,0 +1,781 @@
+#! /usr/bin/python -tt


Is this file being moved from somewhere? If files get moved then xapi.spec might need to be updated too.

It's a copy to get some of the checking working, to be fixed properly in that sub-PR later.

edwintorok · 2024-01-17T14:23:43Z

xcp/compat.py

@@ -0,0 +1,59 @@
+"""Helper module for setting up binary or UTF-8 I/O for Popen and open in Python 3.6 and newer"""


Are these from the xcp-python-libs repo? Perhaps we could use a github checkout action to bring in the right stuff for the CI's purpose (can you run a checkout action with a different repo into a subdir?).
Actually having these files present in XAPI might lead to some code duplication.

Yes, a GitHub checkout from python-libs might be needed,
We'll see how to provide the type-information later in the PR that would do it.

bernhardkaindl · 2024-01-17T15:27:59Z

@edwintorok: Thank you for your kind comments (I answered them now)!

Yes, this PR is just a demonstration, and a proof-of-concept to take bits for production use!

edwintorok · 2024-01-17T15:38:10Z

I assume the last commit (to be split up) can be moved to its own so we could have one PR that adds the CI checks (without modification, or minimal modification to existing python files), and another PR that adds all the other fixes? (which might require wider testing)

liulinC · 2024-01-18T01:42:50Z

scripts/backup-sr-metadata.py

 import atexit
 import XenAPI
 import sys
 import getopt


This file is not packaged by any of xapi packages, nor get installed into dom0,
Is it obsoleted? Maybe better to remove the file.

liulinC · 2024-01-18T01:49:34Z

scripts/examples/python/monitor-unwanted-domains.py

@@ -1,17 +1,37 @@
-#!/usr/bin/env python3
+#!/usr/bin/env python


any reason change this back to python other than python3?
I know if no python2 installed, it goes to python3, but if we updated to python3, does't explicit python3 better?

Here is the code snippet for call_plugin function.

31 let call_plugin session_id plugin_name fn_name args = 32 let plugin_name = find_plugin plugin_name in 33 (* Marshal the args as XMLRPC *) 34 let args = List.map (fun (k, v) -> (k, XMLRPC.To.string v)) args in 35 let call = 36 XMLRPC.To.methodCall fn_name 37 [XMLRPC.To.string (Ref.string_of session_id); XMLRPC.To.structure args] 38 in 39 let output, _ = 40 try 41 Forkhelpers.execute_command_get_output plugin_name [Xml.to_string call] 42 with 43 | Forkhelpers.Spawn_internal_error (log, output, Unix.WSTOPPED _) -> 44 raise 45 | (Api_errors.Server_error 46 | (Api_errors.xenapi_plugin_failure, ["task stopped"; output; log]) 47 | )

Obviously, it does not explicitly specify whether it's using Python 2 or Python 3. This way, it is advisable to explicitly specify the shebang line with python3 to execute these plugins after the py2->py3 upgrade.

P.S. I'm not an OCaml expert, so please correct me if I'm wrong.

I also believe python3 is better here

liulinC · 2024-01-18T03:08:22Z

scripts/hatests

@@ -1,15 +1,15 @@
 #!/usr/bin/env python



This file is not packaged, installed neither. Better to remove.

liulinC · 2024-01-18T03:35:20Z

scripts/restore-sr-metadata.py

@@ -1,26 +1,21 @@
 #!/usr/bin/python
 # Restore SR metadata and VDI names from an XML file


can be removed.

liulinC · 2024-01-18T03:52:41Z

Generally, this is a good PR to enable pytype for python static check.

acefei · 2024-01-18T02:42:00Z

pytest.ini

@@ -0,0 +1,15 @@
+[pytest]


Since you're using pyproject.toml, consider using [tool.pytest] within it instead of a separate configuration file.

acefei · 2024-01-19T09:49:55Z

scripts/examples/python/shell.py

    # We want to support directly executing the cmd line,
    # where appropriate
    if len(sys.argv) > cmdAt:
-        cmd = sys.argv[cmdAt]


Any benefit to rename this variable?

There was some warning about it, either maybe feom pylint (short name) the defaults would reject it.

But it might habe also been a name collision (overload) or a method with the same name.

I would not have been there it if it wasn't a because of a warning.

acefei · 2024-01-19T10:01:59Z

scripts/mail-alarm

+                stdout=subprocess.PIPE
            )
+            chld_stdin = proc.stdin
+            assert chld_stdin


Given the challenges with the debug xapi plugin, it is suggested to raise a more meaningful exception instead of using the assert statement. The code in other files remains the same.

acefei · 2024-01-19T10:48:54Z

scripts/test_usb_scan.py

    @nottest
    def test_usb_common(self, moc_devices, moc_interfaces, moc_results,
                        path="./scripts/usb-policy.conf"):
-        import usb_scan


I noticed you created a separated folder for unit test, do you consider moving all of test_* files into it?

acefei · 2024-01-19T11:25:15Z

Generally, this is a good PR to enable pytype for python static check.

I typically rely on mypy for static type checking, but it falls short in catching the errors (eg: the errors mentioned in PR's description) that Pytype can identify. Meanwhile, I tested with Pyright, it can also do it.

Regarding what @bernhardkaindl mentioned.

When possible, add a check to pre-commit which ensures that files which are touched pass mypy and pyright.

I personally believe choosing either tool is sufficient. (I prefer Pyright)

bernhardkaindl · 2024-01-19T11:49:09Z

Yes, pyright is good too, and you may use it as the default as it's also integrated into vscode But, I would not disregard pytype in addition.

If a combination of all catches one actual bug, that one alone would not have, then use both ar all.

bernhardkaindl · 2024-02-14T19:13:01Z

Closing as to be done later.

bernhardkaindl added 4 commits January 15, 2024 23:24

CI: Add test_usb_reset.py: Test setup_cgroup() of scripts/usb_reset.py

8cd470a

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>

back-out the cancel-in-process config (was for testing)

8a58d2e

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>

re-add-cancel-in-progress

1291881

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>

To-be-split-up: Add pytype to CI to find Py3 bugs and fix issues to l…

7626c33

…et pytype finish Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>

bernhardkaindl marked this pull request as draft January 17, 2024 13:51

edwintorok reviewed Jan 17, 2024

View reviewed changes

bernhardkaindl changed the title ~~Start using pytype (and mypy) to improve the ensure broken Python3 code is not merged~~ Start using pytype (and mypy) to ensure broken Python3 code has a bad time in the future. Jan 17, 2024

liulinC reviewed Jan 18, 2024

View reviewed changes

This was referenced Jan 18, 2024

Add type annotation update pre commit xenserver/python-libs#122

Merged

Still Draft: Big Python3 megapatch, only as Demonstrator of findings from Static analysis #5372

Closed

acefei reviewed Jan 19, 2024

View reviewed changes

acefei mentioned this pull request Jan 24, 2024

CI/Python: Add reporting of code coverage from PyTest #5384

Merged

bernhardkaindl closed this Feb 14, 2024

		@@ -0,0 +1,59 @@
		"""Helper module for setting up binary or UTF-8 I/O for Popen and open in Python 3.6 and newer"""

		@@ -1,17 +1,37 @@
		#!/usr/bin/env python3
		#!/usr/bin/env python

		@@ -1,26 +1,21 @@
		#!/usr/bin/python
		# Restore SR metadata and VDI names from an XML file

Uh oh!

Start using pytype (and mypy) to ensure broken Python3 code has a bad time in the future. #5365

Start using pytype (and mypy) to ensure broken Python3 code has a bad time in the future. #5365

Uh oh!

Conversation

bernhardkaindl commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edwintorok left a comment

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!

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!

acefei Jan 19, 2024 • 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!

bernhardkaindl Jan 19, 2024 • 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!

bernhardkaindl commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edwintorok commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

liulinC Jan 18, 2024 • 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!

liulinC commented Jan 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acefei Jan 19, 2024 • 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!

bernhardkaindl Jan 19, 2024 • 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!

acefei Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

bernhardkaindl commented Jan 17, 2024 •

edited

Loading

acefei Jan 19, 2024 •

edited

Loading

bernhardkaindl Jan 19, 2024 •

edited

Loading

bernhardkaindl commented Jan 17, 2024 •

edited

Loading

edwintorok commented Jan 17, 2024 •

edited

Loading

liulinC Jan 18, 2024 •

edited

Loading

acefei Jan 19, 2024 •

edited

Loading

bernhardkaindl Jan 19, 2024 •

edited

Loading

acefei Jan 19, 2024 •

edited

Loading

acefei Jan 19, 2024 •

edited

Loading