Add Python type annotation for base Compute API #1306
Conversation
|
It turns out this will be quite a challenging and painful task. Existing type annotations I added already uncovered some issues with sub-classes not handling parent types correctly, etc. We don't have much choice here, but to use a type annotation work around in such scenarios (supporting multiple types aka Union), because we can't break the API. In addition to that, it will also be hard to come up with good type annotations for our dynamic |
|
In addition to my comment above - I also decided I will start with an API per pull request to make sure the PRs don't grow too large. In this PR I will work on the compute API and other common code which is needed to get that to work. As far as type annotations for our dynamically returned provider drivers go - I still haven't been able to figure this out. We may just need to go with returning a reference to the base NodeDriver. It's not ideal since it won't know about any provider class specific methods and arguments, but I'm not even sure yet if we can correctly annotate our code which does our dynamic driver class loading (we may need a MyPy plugin or it may even not be possible). |
|
@tonybaloney ^ maybe you have an idea if we can get type annotations to work for our dynamically loaded and returned driver classes. For now, I have this - f5a2016. As mentioned above, it's not ideal since it won't be aware of any provider driver class specific methods and arguments. I did quite a lot of research and experimentation and I couldn't get it to work (aka annotate I believe we may only be able to achieve that using a custom MyPy plugin (but I could be wrong). |
Kami
changed the title
issues detected by MyPy and add missing license headers.
Kami
changed the title
|
This is "mostly" working with limitations mentioned above (get_driver always returns reference to the base driver). I still need to fix enum handling (I needed to change our custom ENUM type so it works correctly with MyPy), then I can hopefully merge it into trunk. |
Those are needed for backward compatibility reasons to ensure it works with old code which treats types as simple strings. Update affected code.
|
I finally managed to get all the tests to pass. Sadly it wasn't that straightforward. One of the main offenders was our custom "enum" type and making sure it works with type annotations and it's also fully backward compatible. For that, I needed to implement a bunch of "glue" code - 9604610 |
Codecov Report
@@ Coverage Diff @@
## trunk #1306 +/- ##
==========================================
- Coverage 86.34% 86.33% -0.01%
==========================================
Files 372 372
Lines 76203 76232 +29
Branches 6976 6979 +3
==========================================
+ Hits 65798 65817 +19
- Misses 7608 7614 +6
- Partials 2797 2801 +4
Continue to review full report at Codecov.
|
|
@c-w maybe you have an idea on how (if possible) to handle that in mypy^ - #1306 (comment) |
|
@Kami I'm not sure this is currently possible in mypy since we only know the type of the returned driver value at runtime based on the provider argument. The current approach looks good to me and may even encourage more users to stick to the methods defined in the base API (a win in my mind since it'll make code more easily cross-cloud portable). We do lose mypy coverage for any driver: MySpecificDriver = get_driver(...) |
|
Thanks. And yes, I believe what you said should work (I will test it and document it). I plan to release release a candidate / beta version with those changes (probably after the next release) to make sure it doesn't break anything. Only thing I'm really worried about is the enum changes I made. |
|
Thanks for contributing to this issue. As it has been 90 days since the last activity, we are automatically marking is as stale. If this issue is not relevant or applicable anymore (problem has been fixed in a new version or similar), please close the issue or let us know so we can close it. On the contrary, if the issue is still relevant, there is nothing you need to do, but if you have any additional details or context which would help us when working on this issue, please include it as a comment to this issue. |
|
I'll work on a patch release first ( |
|
@c-w That's what I came up with - 428c3b6. For some reason, mypy doesn't recognize driver classes (e.g. EC2 = get_driver(Provider.EC2) # type: Type[EC2NodeDriver]
Rackspace = get_driver(Provider.RACKSPACE) # type: Type[RackspaceNodeDriver]The approach I used is not ideal, but I confirmed it works. I'm not sure why it doesn't recognize it (I didn't dig in too much yet), I assume it's something to do with scoping. Might even need to open mypy issue / question about it, but I assume they will come back to us with that our code already relies on too much "magic" and meta programming :D |
|
It turns out this PR inadvertently introduced dependency on I believe I fixed that in 4e42ea3, 5159bab and added a check in 75f07bd. EDIT: Due to libcloud now also depending on enum and typing package, this needed more work - 21a78a1. This approach is much safer anyway. We shouldn't have This should also fix read the docs build. |
|
Just a heads up - the plan is to add |
|
@c-w I just found out today that casting to a driver type doesn't work as expected - python/mypy#3903 :/ MyPy won't inherit type annotations from the base class if subclass overrides the method (it doesn't matter if it preserves the parent method signature). Pretty much only option we have at this point is to add type annotations to all the subclassed methods. This is obviously far from ideal :/ I think only reasonable option we have right now is to simply rely on type annotations from the base driver class. Going forward, we will probably need to use something like this - python/mypy#7548 and also add type annotations to any subclass methods where signature differs from the parent / base class method. |
The goal of this pull request is to add type annotations to the base APIs in an incremental manner.
Adding type annotations to an existing project is much harder (and takes much more time) compared to adding it as you develop a new project and that's why I will take an incremental approach.
I plan to start with the base APIs and see how it goes. It's likely that those type annotations will require various issues in the driver code which will be hard and time consuming to fix (and that's also the reason why I plan to start with the base APIs).