FastAPI port of API/Studio, second time around

[dhdaines]

This time with backwards compatibility and testing! Wow!

[codecov]

Codecov Report

Attention: Patch coverage is 98.63014% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 93.84%. Comparing base (7ec44b3) to head (9bc3855).

❗ Current head 9bc3855 differs from pull request most recent head 2d68577. Consider uploading reports for the commit 2d68577 to get more accurate results

Files	Patch %	Lines
g2p/app.py	97.43%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #360      +/-   ##
==========================================
+ Coverage   93.77%   93.84%   +0.07%     
==========================================
  Files          16       16              
  Lines        2328     2291      -37     
  Branches      517      516       -1     
==========================================
- Hits         2183     2150      -33     
+ Misses         82       79       -3     
+ Partials       63       62       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

CLI load time: 0:00.05
PR head 2d6857776b0dd4bd0d2d8dd995c780ab60ea34fb
Imports that take more than 0.1 s:
import time: self [us] | cumulative | imported package

[dhdaines]

Note that fastapi-socketio is really broken, so it would probably be better to not use it: pyropy/fastapi-socketio#52

In the end there's very little actual code in it so we can just use socketio directly.

[dhdaines]

Also note that this would mean that the API and Studio require Python 3.8, so not sure if we want to go there...

[joanise]

Also note that this would mean that the API and Studio require Python 3.8, so not sure if we want to go there...

The Web API already requires Python 3.8, we only continue to support 3.7 for the programmatic Python API, i.e., g2p as a Python module.

[dhdaines]

Also note that this would mean that the API and Studio require Python 3.8, so not sure if we want to go there...

The Web API already requires Python 3.8, we only continue to support 3.7 for the programmatic Python API, i.e., g2p as a Python module.

Ah, of course - then there's no problem since the API is a separate, optional dependency here. But we need to separate the test suites so that we can test the non-API parts with 3.7.

[dhdaines]

Also, not quite sure what's going wrong with coverage in CI here, it gives the correct results when I run it locally.

[joanise]

I really like these changes, although on my end g2p-studio is broken on this branch, I don't know if I'm doing something wrong or if it's a bug.
I better get my act together and review your hatch PR, but having installed using hatch for this branch I'm already pretty much on-board now.

CI coverage: I don't know what's going on.

I've left notes about the clunky way I've been making sure we're still exercising Python 3.7 in CI. I'm open to better suggestions: the actual goal is that some part of CI should exercise g2p as a Python module and CLI command (i.e., all the tests that don't involve g2p-studio or the REST API) with Python 3.7. I'm not attached to how we get there, just that we do it. And that's a hint that my documentation was sorely inadequate, nothing in there pointed out that goal.

[joanise]

Here this is conflating the migration from 1.x to 2.0 and from 2.0 to what will probably have to be 2.1 or possibly even 3.0 if this is not backwards compatible. While I don't think we need a separate migrating to 2.1 file, I think this section should distinguish between migrating to 2.0 (previous text) and to 2.1 new text like you're writing here.
Side note: losing routes is unfortunate. But I assume there's some other form of auto-generated API documentation?

[dhdaines]

well, there's the auto-generated online API documentation, which is arguably more useful anyway...

[joanise]

I believe this will be ready to merge, with #363 and a redirect link from /docs to /api/v1/docs, although when we also merge the v2, we might want a page listing both instead of an actual redirect...

[joanise]

Meh, I'm being bitten by a stale environment again. I'll create something fresh to test.

[joanise]

I hate dependency hell!

[dhdaines]

I hate dependency hell!

Hm, I'm really surprised this is happening ... I didn't have to change anything in my environment, the packages are the same as before the last updates except that we aren't using some of the ones we were previously...

[joanise]

Found it:

     "starlette>=0.35.1",
-    "python-socketio>=5",
+    "python-socketio>=5.9.0",
     "uvicorn",

[dhdaines]

Found it:

     "starlette>=0.35.1",
-    "python-socketio>=5",
+    "python-socketio>=5.9.0",
     "uvicorn",

aaah ... okay, I put python-socketio>=5 because that's what supports the same protocol as the JavaScript side, but I'll apply this fix (presuming there is no problem with Python 3.8)

[joanise]

That's the minimum required patch. On a stale environment with python-socketio==5.8.0 I got exceptions like this from ./run_studio.py:

WARNING:  No supported WebSocket library detected. Please use 'pip install uvicorn[standard]', or install 'websockets' or 'wsproto' manually.
INFO:     127.0.0.1:62614 - "GET /ws/socket.io/?EIO=4&transport=websocket HTTP/1.1" 500 Internal Server Error
ERROR:    Exception in ASGI application
Traceback (most recent call last):

A simpler fix was to install uvicorn[standard], but that installed quite a few more things. Just python-socketio==5.9.0 gave me simple-websocket==1.0.0 as a side effect and that was enough.

I suppose we could also declare simple-websocket ourselves in the api dependency block.

[joanise]

Warning: you accidentally undid #365. Maybe we need to make the change in pyproject.toml too. That fixes a prod CVE, so we really want to apply it.

[dhdaines]

Warning: you accidentally undid #365. Maybe we need to make the change in pyproject.toml too. That fixes a prod CVE, so we really want to apply it.

Oh, oops. This may be due to "git rebase"'s merge conflicts being backwards and hard to understand.

[dhdaines]

Warning: you accidentally undid #365. Maybe we need to make the change in pyproject.toml too. That fixes a prod CVE, so we really want to apply it.

Oh, oops. This may be due to "git rebase"'s merge conflicts being backwards and hard to understand.

I don't understand, though ... how did the updated gunicorn dependency disappear from the main branch?

[joanise]

Alright, finally I can say I agree: we're there! We can merge this.

Thank you for your patience working through all the details, and putting up with my stale environments which, while annoying, bring us to express our actual dependencies more accurately, so I don't actually feel bad about it. 😉

[joanise]

🎉

[joanise]

Oh, oops. This may be due to "git rebase"'s merge conflicts being backwards and hard to understand.

I don't understand, though ... how did the updated gunicorn dependency disappear from the main branch?

You almost certainly made a mistake in handling a merge conflict.

This is the commit that undoes it:

commit cfc50c6ef1
Author: David Huggins-Daines <dhd@ecolingui.ca>
Date:   Thu Apr 4 10:09:02 2024 -0400

    fix: update prod environment and workflow

[joanise]

In your defense, merge conflict resolution is indeed quite confusing. I'm quite paranoid about them, often doing extensive reviewing of the commit I'm trying to rebase and the changes that were there before, and not just relying on the conflict resolution tools.