Add type annotations

[chadrik]

This is a first pass at adding type annotations throughout the code-base. Mypy is not fully passing yet, but it's getting close.

Addresses https://github.com/AcademySoftwareFoundation/rez/issues/1631

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: chadrik / name: Chad Dombrova (ea166f5d5422f33d1c43816e288e6baefe4e5155, dd8619291ba7db0bbd52259645d058afb570c1d3)

[chadrik]

Protocol and TypedDict are not available in the typing module until python 3.8.

We have a few options:

1. vendor typing_extensions
2. remove use of these typing classes until Rez drops support for python 3.7
3. I can create a mock of Protocol and trick mypy into using it which is safe because it has no runtime behavior. Doing the same thing for TypedDict is more complicated, but possible.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 83.19328% with 180 lines in your changes missing coverage. Please review.

Project coverage is 59.16%. Comparing base (491497f) to head (dd86192). Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/rez/suite.py	53.33%	20 Missing and 1 partial :warning:
src/rez/version/_version.py	88.81%	11 Missing and 7 partials :warning:
src/rez/package_order.py	73.43%	13 Missing and 4 partials :warning:
src/rez/resolved_context.py	75.92%	11 Missing and 2 partials :warning:
src/rez/solver.py	94.03%	10 Missing and 3 partials :warning:
src/rez/package_resources.py	77.55%	9 Missing and 2 partials :warning:
src/rez/build_system.py	65.51%	9 Missing and 1 partial :warning:
src/rez/packages.py	82.75%	9 Missing and 1 partial :warning:
src/rez/build_process.py	75.00%	5 Missing and 2 partials :warning:
src/rez/utils/data_utils.py	79.31%	4 Missing and 2 partials :warning:
... and 23 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1761      +/-   ##
==========================================
- Coverage   59.30%   59.16%   -0.15%     
==========================================
  Files         126      127       +1     
  Lines       17210    17452     +242     
  Branches     3015     3049      +34     
==========================================
+ Hits        10206    10325     +119     
- Misses       6319     6408      +89     
- Partials      685      719      +34     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[chadrik]

I got bored and added lots more, particularly focused on the solver module. Once the solver module is complete, we can experiment with compiling it to a c-extension using mypyc, which could provide a big speed boost!

[chadrik]

I now have rez.solver compiling as a C-extension with all tests passing. I'm very interested to see how the performance compares. Does anyone want to volunteer to help put together a performance comparison? Are there any known complex collection of packages to test against?

[chadrik]

I found rez-benchmark. Interestingly, rez is slower with the compiled rez.solver. It could be because there are many modules and classes used by rez.solver which have not been compiled.

I probably won't have time to dig into this much more, but once this PR is merged I'll make a new PR with the changes necessary for people to test the compiled version of rez.

[chadrik]

Note: this PR likely invalidates https://github.com/AcademySoftwareFoundation/rez/pull/1745

[chadrik]

@instinct-vfx Can you or someone from the Rez group have a look at this, please?

[JeanChristopheMorinPerso]

@chadrik You need to sign the CLA before we can even start to look at the PR.

[chadrik]

@JeanChristopheMorinPerso

@chadrik You need to sign the CLA before we can even start to look at the PR.

I work for Scanline, which is owned by Netflix, and I'm meeting with our CLA manager on Monday. I made this contribution on my own time: can choose individual vs corporate CLA on a per PR basis?

[JeanChristopheMorinPerso]

I made this contribution on my own time: can choose individual vs corporate CLA on a per PR basis?

I don't think you "can" but your account can be associated to an an ICLA and a CCLA. But I'm not a lawyer so I can't help more than that. If you and or your employer/CLA manager have questions, you can contact the LF support by following the link in the EasyCLA comment: https://github.com/AcademySoftwareFoundation/rez/pull/1761#issuecomment-2119309923.

[chadrik]

CLA is signed!

[chadrik]

Any thoughts on this PR?

[JeanChristopheMorinPerso]

Hey @chadrik, I made a first good read last week and I'll try to do another one soon. If you have the time, I would really love to see a GitHub Actions workflow that would run mypy on all pull requests.

[chadrik]

I would really love to see a GitHub Actions workflow that would run mypy on all pull requests.

Me too!

The challenge is that there are still a lot of errors. These are not the result of incorrect annotations, but rather due to code patterns which are difficult to annotate correctly without more invasion refactors. For example, there are quite a few objects which dynamically generate attributes, and that's a static typing anti-pattern.

If you'd like an Action that runs mypy but allows failure for now, that's pretty easy, but if you want failures to block PRs, that'll take a lot more work. I'd prefer not to make that a blocker to merging this, though, because I've had to rebase and fix merge conflicts a few times already.

I do have a plan for how we can get to zero failures in the mid-term: I wrote a tool which allows you to specify patterns for errors to ignore, but I need to update it.

[JeanChristopheMorinPerso]

I think we can introduce a workflow that will fail for newly introduced errors and warnings. I'm sure someone already thought of that somewhere and we can probably re-use what they did?

Basically, I'd like to verify that your changes work as expected and that we don't regress in the future and that new code is typed hint. Mypy can also be configured on a per module basis right?

[chadrik]

@JeanChristopheMorinPerso I used a project called mypy-baseline to create a set of ignore patterns that can be filtered out until they're resolved: https://mypy-baseline.orsinium.dev/usage

If new errors are introduced they will need to be resolved (i.e. errors not filtered by the established baseline regexes). The regexes are bound to particular files but not to line numbers. The mypy CI job will pass if a developer fixes an error, but in this case it is good practice to set a new baseline using mypy | mypy-baseline sync --ignore-categories=note. When creating a baseline, care must be taken to use the same version of python and mypy.

[dbr]

Not sure if it was discussed already, but is it feasible to only use # typing: ignore comments to suppress existing known errors?

Main benefit would be people using mypy integration in their editors wouldn't be flooded with the errors ignored by mypy-baseline

I think mypy also warns when the ignore is no longer needed, so if some refactoring resolves the issue, the comments are easy to clean up

[chadrik]

There are about 300 errors, so using inline ignores has the downside of littering the code with type comments. It also means that developers need to be careful to move the ignore comments to the proper locations when refactoring code, and the solution not always obvious. I think at this stage what I’ve presented here is a very low effort way to ease this project into type annotations. With the mypy-baseline project there is a simple way to just bulk add problems to the ignore list, if necessary.

FWIW, most of the remaining errors will require some pretty major refactors to resolve — meaning we probably need to replace the schema objects with something like dataclasses or pydantic to remove dynamic attribute generation. If folks are interested in taking on some more substantial changes to get the code base fully covered, I’m happy to do that.

[chadrik]

Shall we get this merged?

[chadrik]

Hi! Is there anything I can do to get this across the line?

[JeanChristopheMorinPerso]

Rebased to get all the latest changes from main.

[chadrik]

It would be great if all this work didn't go to waste. If I rebase this, can we get it merged?