Document Python type annotations and checks.

[TimothyEDawson]

Changes proposed in this pull request

• Adds developer documentation for Python type annotations, including:
  • Adding type check related optional dependencies to the example environment.yaml file.
  • Describing the current scope of mandatory type annotations (external interfaces).
  • Describing the purpose of each type checking step.
  • Showing how to run the type checking steps locally.

If applicable, fill in the issue number this pull request is fixing

Closes Cantera/enhancements#251

AI Statement (required)

• No generative AI was used. This contribution was written entirely without AI assistance.

Checklist

• The pull request includes a clear description of this code change
• Commit messages have short titles and reference relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• Style & formatting of contributed code follows contributing guidelines
• AI Statement is included
• The pull request is ready for review

[TimothyEDawson]

Note: The lxml dependency is required by mypy[reports] (and not by the core mypy), but for some reason conda doesn't seem to install this automatically. Not sure what's up with that? Ideally it wouldn't appear in the environment.yaml at all.

[bryanwweber]

Thanks Tim! I have a few fairly small comments. The only bigger one is to add some examples where we have # type: ignore and what their justification is.

[TimothyEDawson]

Alright, I think we're a lot closer now!

[ischoegl]

Thanks, @TimothyEDawson, for tackling this. I have independently played with type annotations in my own code bases and wanted to share some of my conclusions:

• .pyi files are a very limited band-aid. Other than convenience to third party software or end-users of our package there is no benefit to code quality (i.e., I am challenging the 'secondary benefit' you mention in your description; rather, .pyi stub files make code maintenance harder).
• Direct type annotations in .py files is a different subject matter, as there is an actual analysis happening. There's the obvious caveat of Cython in its current form.
• It has been my experience that making mypy/pyright pass for .py is relatively straightforward (as in: can be taken care of by coding agents). Adding stubtest runtime checks into the mix, however, makes things dicey, and involves exceedingly ugly hacks (--allowlist) for silly reasons.
• If we are concerned about code quality, I'd rather like to see ruff check than tracking down ways to make mypy/pyright plus stubtest happy. It's doable to add some of this in with stubs retroactively into our codebase (as has been done), but is not reasonable with .py. I'd rather wait until those tools get better.

[TimothyEDawson]

• .pyi files are a very limited band-aid. Other than convenience to third party software or end-users of our package there is no benefit to code quality (i.e., I am challenging the 'secondary benefit' you mention in your description).
• Direct type annotations in .py files is a different subject matter, as there is an actual analysis happening. There's the obvious caveat of Cython in its current form.

I think you've misread. The stub files are not where the secondary benefit applies, they only apply to the native Python code. In fact, that's the entire point of that paragraph.

• It has been my experience that making mypy/pyright pass for .py is relatively straightforward (as in: can be taken care of by coding agents). Adding stubtest runtime checks into the mix, however, makes things dicey, and involves exceedingly ugly hacks (--allowlist) for silly reasons.

Sure, but stubtest is the only way to verify stubs. Without it, you can put whatever you want in the stubs with no regard for actual runtime behavior, and only manual checks would catch errors and mistakes. As long as we have stub files we need stubtest, and as long as we have Cython-syntax code we need stub files.

• If we are concerned about code quality, I'd rather like to see ruff check than tracking down ways to make mypy/pyright plus stubtest happy. It's doable to add some of this in with stubs retroactively into our codebase (as has been done), but is not reasonable with .py. I'd rather wait until those tools get better.

Ruff is a linter, not a type checker. It is not a replacement for mypy/pyright in any sense. Hence why Astral (developer of ruff) is currently working on ty, which could be a replacement, and will have all of the same issues.

Two side notes:

1. I would very much like to add ruff to the Cantera workflow, as I have been using it extensively. I can open an enhancement issue and/or pull request for this.
2. Some people have just started discussions about type-checking Cython code on both the ty and pyrefly Issue trackers, although I am not holding my breath on these.

[ischoegl]

Agreed that ruff is a linter. But their ANN option, in combination with mypy, surfaced quite a few issues. Good to hear about ty and other potential future options for Cython. Those stubfiles need to go - for me as a developer, there is no benefit - having type hinting part of py is a real benefit as I can see what type to work with at the interfaces. For stubfiles, there is just extra maintance burden.

I'll grudgingly agree on stubtest being a necessary evil, but would like to see it moved to post-merge-tests. Any inexperienced Python programmer trying to contribute will be chasing their tail.

[bryanwweber]

Thanks again @TimothyEDawson I had a chance to pull this down and try the commands, so I have a few suggestions for clarifying and making this more self contained. I think that will make for a better experience for devs who might not be as familiar with Python type hinting.

[bryanwweber]

This produced a bunch of error outputs in the with_units interface, is that expected?

[TimothyEDawson]

Yes, I haven't attempted to get Pyright passing as it tends to be much more strict than Mypy. It prints error messages during this check, but it does not actually error, and only the last bit about the coverage is the relevant output of the --verifytypes command.

[bryanwweber]

I appreciate listing all the type checkers here, but we really only support mypy and pyright, right? At least, I tried to run ty and got a bunch of errors. I think listing everything is confusing if we don't support those type checkers, so I think we should remove those links. I think it would be helpful to show the CLI commands for the other checkers, but I don't expect you to handle that.

[TimothyEDawson]

In the future, once we have more of the code actually being checked at all (i.e. if we can convert the Cython syntax to Python syntax), I think it would be great to include all of these type checkers in the CI. In the interim I think it's worth mentioning them to show they've been considered, and let contributors know they exist.

It's also good practice for contributors to try multiple checkers and get their code passing, even when Mypy is the only one currently enforced. I use Pyrefly quite a lot, myself. I don't think that's necessarily worth recommending in the developer guide, though.

[bryanwweber]

I see this link here, but it's easy to miss. I think we should describe one way to do everything in this file (i.e., if any steps require the built library, we should set it up so all of them use the built library) and include any necessary configuration (e.g., exporting environment variables) as code snippets in this doc page, in addition to this link. I know that will be some duplication, but typing is esoteric enough that I want to make this page as easy to understand on its own as possible.

[TimothyEDawson]

I'm open to laying out the directions more explicitly here, it's a good idea. I do think it's important to be explicit about which checks don't require a local build at all, as that has a significant impact on the developer workflow. Specifically, you really shouldn't be rebuilding between checks, as it adds an unnecessary step and extra time. Static type checking should be performed in a tight loop during development, and ideally most of it is happening as you type using a language server. Meanwhile, the checks on built libraries are a necessary evil right now, but mostly just when dealing with the stub files, and hopefully they can be eliminated in the future.

[bryanwweber]

Specifically, you really shouldn't be rebuilding between checks, as it adds an unnecessary step and extra time... ideally most of it is happening as you type using a language server.

Please correct me if I'm wrong, but the only way to check the runtime types is by rebuilding until and if we are able to put hints into Cython? Insofar as this is a guide for developers of Cantera and not developers using Cantera, I really think we should focus on the steps that require the build step and just accept that it's not ideal right now.

[bryanwweber]

I mentioned this in the previous comment, I think this is the only way we should show to users. I could maybe be convinced that we can show the "no built library" in an expandable tab if you really want to keep it.

[TimothyEDawson]

I would argue the "no built library" is the only one which should be shown.

[ischoegl]

@bryanwweber / @TimothyEDawson … what is the status on this PR?

[bryanwweber]

Thanks for the ping Ingmar! I'm waiting for Tim to reply to me or make the suggested changes. ☺️