Transcribe: Enable MyPy #12588
Transcribe: Enable MyPy #12588
Conversation
3e9da18 to
14069d2
Compare
|
@sannya-singal As a service owner for transcribe: What do you think? I personally are very much in favor or increasing the enforcement of type hints and checks, but it's really you as a service owner who has to decide if you want to introduce this for your service. 😃 |
There was a problem hiding this comment.
Awesome work 🚀 Enabling MyPy for the Transcribe module is a great step towards our broader goals of strengthening type checks across the codebase, and I really like the incremental approach you are taking here 🎉
Kudos as well for integrating this with the first AWS service 🙌 Having type checking enabled at the service level will also empower code owners to enforce typing in their codebase, also making it easier to catch issues early and improve maintainability.
Looks great from my side 🚀 thanks for pushing this forward! 👍
There was a problem hiding this comment.
Thanks again for this push towards better and enforced typing! 💯
I just stumbled upon the exclusion of the updated provider from the unit test which worries me a bit. I think it would be great to quickly discuss how we want to move forward here before we merge this PR.
| next_token: NextToken = None, | ||
| max_results: MaxResults = None, | ||
| **kwargs, | ||
| status: Optional[TranscriptionJobStatus] = None, |
There was a problem hiding this comment.
suggestion: We could maybe directly use PEP 604 syntax here?
| status: Optional[TranscriptionJobStatus] = None, | |
| status: TranscriptionJobStatus | None = None, |
There was a problem hiding this comment.
I explicitly used Optional here because it's not supported in Python 3.9, so if the CLI ever explicitly loads this module in Python 3.9 it would fall over.
I don't know how likely that is though - I'm happy to do either way 🤷
| if "TranscribeApi" in str(base_class): | ||
| raise SkipTest( | ||
| "Transcribe Signatures are different on purpose, as optional args are marked as such" | ||
| ) |
There was a problem hiding this comment.
suggestion: Unfortunately, this one is problematic. This test makes sure that the provider implementations don't drift from the upstream API / their superclasses.
While we did reduce the coupling between the generated API and the providers implementation with adding the kwargs everywhere, we were still monitoring and updating the signatures based on this tests' results.
An example for an ASF Update PR with changed providers would be #12490.
The root of this problem is actually that we do generate the code with wrong typing. In this case I think we should just update the code generation and all its subclasses. It might even make sense to implement a script (with something like libcst) that does this automatically to help us keep our implementations up-to-date automatically (as part of the ASF Update action directly).
What do you think?
There was a problem hiding this comment.
Updating the generated code makes a lot of sense IMO. Considering the scope of changing signatures over the entire codebase, we could split it up in three parts:
- Change this test to view
X=NoneandX|None = Noneas equal, as a stop gap measure while we have both versions in place - Change the ASF update action to update all the generated code to
X | None - Change the providers themselves using
libcst
We could do all of this in one PR, but that would be massive, and very difficult to review.
Any objections to the stepwise approach @alexrashed?
(I'll mark this PR as draft for now, until we have a solution.)
There was a problem hiding this comment.
This is perfect. let's move forwards with (1) here and then we can tackle the other two in follow ups! 💯
14069d2 to
3332882
Compare
3332882 to
212965f
Compare
There was a problem hiding this comment.
LGTM 🚀 Thanks for incorporating the feedback! I will defer to @alexrashed for a re-review since he requested the changes.
Motivation
Part of a broader push to enforce typing support everywhere (see #12532). One of the areas that I wanted to focus next on was adding correct types to the
BaseStore/AccountRegionBundle/etc, because it is such a fundamental concept that all emulators would benefit from improved type support.However, before improving types there, it makes sense to first ensure that the usage of
BaseStoreis typed and validated. Only then can we be improve the types inBaseStoreand be sure they are correct.Hence this PR: enforcing type checks in one of the smaller services, for the sole purpose of checking that
BaseStoreis used/typed correctly.History
param: X | None = None) were added in ASF: Mark optional params as such (X | None) #12614context.account_id/region_namewere set in Core: Add type hints to aws/core.py #12617An earlier version of this PR tackled everything at once, but with the above PR's already in place, this PR has been simplified to only change the Transcribe-specific types.