Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autogenerate types with pyre and check with mypy 3.8 #214

Merged
merged 13 commits into from
Feb 9, 2023

Conversation

bolasim
Copy link
Collaborator

@bolasim bolasim commented Feb 8, 2023

What

Truss needs to be well typed for developer ergo and productivity. It's also very helpful for catching a lot of compile-type issues earlier that we can't do with python

How

  1. Autogenerate types with pyre (code no included, was one time operation)
  2. Add mypy to do type checking
  • We configure this to check for types in python3.8 style since that's the lowest supported python version
  1. Add mypy to the pre-commit checks for lint to run both locally and in CI

Notes:

  • I ignored all the test files and the test_data files for now. We can discuss adding those in a later PR
  • I ignored all the model.py files
  • We should talk about making a baseclass for truss.Model and typing properly instead of doing all this funny hasattr stuff all over the place

Testing

The hope is the unit and integration tests catch everything here

@bolasim bolasim force-pushed the bola/add-types-check branch 2 times, most recently from 8d8c202 to 4c9bcde Compare February 8, 2023 17:31
@zero1zero
Copy link
Contributor

Excited for this!

@bolasim bolasim force-pushed the bola/add-types-check branch 2 times, most recently from 810e694 to 4861f37 Compare February 9, 2023 11:35
@bolasim bolasim marked this pull request as ready for review February 9, 2023 11:56
@bolasim bolasim force-pushed the bola/add-types-check branch from b8928ac to 79ab057 Compare February 9, 2023 11:58
@bolasim bolasim requested a review from howespt February 9, 2023 12:00
@pankajroark
Copy link
Collaborator

About

We should talk about making a baseclass for truss.Model and typing properly instead of doing all this funny hasattr stuff all over the place

The reason for not having a baseclass for the model class in Truss is that we didn't want the user truss to depend on an additional library, certainly not Truss. Bundling an extra class into the default truss takes away from the simplicity. Open to ideas. Perhaps when we carve out a smaller base library out of Truss, which could have this base class, we can have the user truss depend on it.

@bolasim
Copy link
Collaborator Author

bolasim commented Feb 9, 2023

Sounds good. Looking forward to chatting about some of this truss later today.

I'm re-running integration tests here and optimistic they'll pass: https://github.com/basetenlabs/truss/actions/runs/4135323088/jobs/7147618276

Copy link
Collaborator

@pankajroark pankajroark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement!

@bolasim bolasim merged commit c182964 into main Feb 9, 2023
@bolasim bolasim deleted the bola/add-types-check branch February 9, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants