Replace cast(...) in generated Python code with type annotations

[ntamas]

This patch removes the occurrences of cast(some_type, expr) in the generated Python code and replaces these expressions with type annotations. The patch increases the speed of the MAVLink parser from about 93K packets/sec to 111K packets/sec in my benchmarks on a Macbook Pro M1 while still maintaining type safety (all mypy checks pass).

The reason for the runtime cost of cast(some_type, expr) is that cast() is a proper function in Python that simply ignores the first argument and returns the second argument. cast() only has a special meaning for type checkers. However, some_type (the first argument) still needs to be constructed when cast() is called even though it will be discarded immediately, and the cost is non-negligible for complex types like Tuple[bytes, int, int, int, int, int, int, int, int]. In contrast, inline type annotations used in this patch are completely stripped away when the source is compiled into bytecode so they have zero runtime cost.

[ntamas]

CI failure in the test pymavlink / build (3.7) job is due to an outdated mypy in that environment; newer versions of mypy are smarter and accept the generated code without problems. Not sure whether it's worth trying to work around this issue - let me know what you would prefer.

[cclauss]

Impressive. I had heard about this issue but had never seen anyone benchmark it. Are your packets/sec benchmarks in this repo, or is that custom code that you created? I find the proposed changes much easier to read. I like that several lines have been moved outside the try / except block.

I am puzzled why ruff check --select=TC006 does not detect these as issues, but that is separate from this pull request.

I am not a maintainer, so please wait for other reviews, but if a maintainer agrees with your changes, then we should disable mypy on legacy Python 3.7 by replacing:

pymavlink/.github/workflows/test.yml

Line 60 in 0e3222c

mypy --config-file ./.github/workflows/mypy-generated.ini dialects/v10/*.py dialects/v20/*.py

with

+
+      - name: Type check generated python code
+        if: matrix.python-version != '3.7'
+        run: |
+            mypy --config-file ./.github/workflows/mypy-generated.ini dialects/v10/*.py dialects/v20/*.py

And

pymavlink/.github/workflows/mypy-generated.ini

Line 2 in 0e3222c

python_version = 3.6

+ python_version = 3.8

[ntamas]

Are your packets/sec benchmarks in this repo, or is that custom code that you created?

Custom code; not secret at all but it requires a large pickle file (about 68 MB) that contains actual signed MAVLink packets plus noise (fragmented packets, serial line noise etc) that we captured from ~1000 drones running at a same time on an UDP/IP network, and the benchmark kinda doesn't make sense without real data. I can publish it somewhere if needed. Usually the casts don't cause a problem in normal Python code, but this was special because it was on a hot path and contained a complex type signature.

[tridge]

Nice work! I'm happy with disabling mypy on 3.7

[cclauss]

🚀

[peterbarker]

@ntamas we try to avoid merge commits in all of our repos... would be nice to have this rebased on master.

[ntamas]

@peterbarker Done.

[tridge]

@ntamas sorry, it has conflicts, I github is not allowing me to rebase it (maybe you unticked the allow maintainer edits button?)

[ntamas]

maybe you unticked the allow maintainer edits button?

No, I didn't, but it's a cross-organization pull request (our fork is under an org account) so GitHub does not show this checkbox.

Conflicts fixed - would you like me to rebase to get rid of the merge commit?

[tridge]

Conflicts fixed - would you like me to rebase to get rid of the merge commit?

yes please, and the PR still shows conflicts for me "This branch cannot be rebased due to conflicts"