Add comprehensive type annotations #566
Conversation
Type cleanup
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
| return deploy_app(app_mode=AppModes.PYTHON_API, **locals()) | ||
|
|
||
|
|
||
| def deploy_python_fastapi( |
There was a problem hiding this comment.
This function is imported by vetiver-python and its removal may be why the tests are failing.
There was a problem hiding this comment.
Ah, that's good to know! This makes me a little concerned, though: I removed many of the functions in actions.py because they were not used internally and seemed to essentially be duplicates of functions in main.py. Do we need to keep any other of those functions around in case someone external was using them?
Also, in this particular case, I wonder if we can just have actions.deploy_python_fastapi be an alias to main.deploy_python_fastapi.
There was a problem hiding this comment.
Currently the only non-CLI clients are vetiver-python and rsconnect-jupyter.
There was a problem hiding this comment.
I wonder if we can just have
actions.deploy_python_fastapibe an alias tomain.deploy_python_fastapi
Yes, as long as we're careful to match the type signature that Vetiver is expecting.
There was a problem hiding this comment.
Hm, there currently isn't a main.deploy_python_fastapi; the command line command is generated by the following and isn't saved in a variable anywhere:
generate_deploy_python(app_mode=AppModes.PYTHON_FASTAPI, alias="fastapi", min_version="2021.08.0")Also, the function signature is somewhat different and I don't want to risk breaking things, so for now I'll just bring back this code.
|
From some testing I did in #571, it looks like commit c476d69 made the test break. It looks like something on the server side stopped working? In that commit I added many type annotations, but didn't make any functional changes. I suppose it's possible that some of the syntax I used there was not compatible with Python 3.8. This is the line that cause the error: As far as I can tell, that code does not cause |
|
Now when I run the tests, the results are inconsistent. For example, on the same commit, I ran the workflow twice, and the failure happened differently between the two runs.
I don't understand why the first one failed, but the second run failed in a way that makes sense to me and that I can fix. |
|
I fixed the bug mentioned in the previous comment, and now here's a case where it fails on the first run and succeeds on the second:
I see that in #569, where the command was switched from
I suspect there's a timing problem with line |
This PR follows up on a discussion I had with @mmarchetti. It does the following:
In the changes here, there are still many, many issues flagged by pyright in strict mode.
The purpose of this is to set things up for a second pull request, which will be made against this pull request instead of master. The second PR will have changes that are not quite as safe as this PR -- it will add more type annotations and makes some changes to the logic, and is therefore not as safe as this PR. The reason they're done with two separate pull requests is so that the substantive changes in the second PR are more visible and not drowned out by the changes in this PR, which are safe. This will make the second PR easier to review.
After the second PR is merged into this one and tests are passing, we can merge this PR into master.
The overall goal is to get the code passing with pyright in strict mode in CI, and disable mypy. Once this is all done, it will be much easier to contribute to this repository and refactor the code here.