search

AcademySoftwareFoundation / rez

An integrated package configuration, build and deployment system for software
https://rez.readthedocs.io
Apache License 2.0
916 stars 328 forks source link

Add type annotations #1761

Open chadrik opened 1 month ago

chadrik commented 1 month ago

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] commented 1 month ago

CLA Signed

The committers listed above are authorized under a signed CLA.

chadrik commented 1 month ago

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] commented 1 month ago

Codecov Report

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

Project coverage is 58.26%. Comparing base (e215a77) to head (efcae39). Report is 3 commits behind head on main.

Files 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 ```diff @@ Coverage Diff @@ ## main #1761 +/- ## ========================================== - Coverage 58.39% 58.26% -0.13% ========================================== Files 126 127 +1 Lines 17205 17447 +242 Branches 3519 3568 +49 ========================================== + Hits 10047 10166 +119 - Misses 6491 6580 +89 - Partials 667 701 +34 ```

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

chadrik commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

chadrik commented 1 month ago

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

JeanChristopheMorinPerso commented 2 weeks ago

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

chadrik commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 6 days ago

CLA is signed!