search

roedoejet / g2p

Grapheme-to-Phoneme transductions that preserve input and output indices, and support cross-lingual g2p!
https://g2p-studio.herokuapp.com
Other
119 stars 26 forks source link

API v2, second time around #359

Closed dhdaines closed 3 months ago

dhdaines commented 3 months ago

Here's the new API used in the alignment demo (https://github.com/dhdaines/alignment-demo and https://aligner.ecolingui.ca/), updated for G2P 2.x. There's also a small fix to the substring alignment code, can't quite remember what it was for but it should have coverage in any case. (EDIT: Nope actually it doesn't have coverage ... well, that will have to get fixed)

For the moment I made fastapi an optional dependency, so this should have no effect on anything else. It is based on dev.hatch though, at least for the moment.

github-actions[bot] commented 3 months ago 
CLI load time: 0:00.05
PR head c214c6fd8d95e350d9dab76e54012acb89dc0509
Imports that take more than 0.1 s:
import time: self [us] | cumulative | imported package
codecov[bot] commented 3 months ago

Codecov Report

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

Project coverage is 93.36%. Comparing base (2d68577) to head (c214c6f). Report is 1 commits behind head on main.

Files Patch % Lines
g2p/api_v2.py 84.84% 14 Missing and 6 partials :warning:
g2p/transducer/__init__.py 25.00% 1 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #359 +/- ## ========================================== - Coverage 93.84% 93.36% -0.48% ========================================== Files 16 17 +1 Lines 2291 2427 +136 Branches 516 542 +26 ========================================== + Hits 2150 2266 +116 - Misses 79 92 +13 - Partials 62 69 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

joanise commented 3 months ago

Initial clarification question: is the intent to ultimately merge both #360 and this PR, i.e., to update API v1 to use FastAPI, and to add this API v2?

dhdaines commented 3 months ago

Initial clarification question: is the intent to ultimately merge both #360 and this PR, i.e., to update API v1 to use FastAPI, and to add this API v2?

Yes, they are independent of each other, but the idea would be to add both APIs (/api/v1 and /api/v2)

dhdaines commented 3 months ago

Great! Way easier to review this one, now that the hard work was done in the other PR!

Thanks! I'm not going to worry too much about the coverage issues for the moment...