feat: DIA-953: Stream results from adala inference server into LSE #75
Conversation
matt-bernstein
commented
Mar 27, 2024
•
matt-bernstein
commented
- add Label Studio SDK as a dependency (should be an optional dependency for the server only, but there's no clean way to do this in poetry)
- switch ResultHandlers from an enum whitelist of functions to a registry of Pydantic models, same implementation as Environment/Skill/Runtime
- add LSEHandler that pushes predictions to an LS project using an LS SDK Client
- change celery worker functions to use pickle serialization, removing manual step of de/serializing objects on each worker call from an API endpoint
- change utils and handlers import paths to be usable from both main app and celery workers
- make AsyncKafkaEnvironment instantiable as prep for post-streaming refactoring
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #75 +/- ##
==========================================
- Coverage 50.28% 46.28% -4.01%
==========================================
Files 37 39 +2
Lines 1587 1735 +148
==========================================
+ Hits 798 803 +5
- Misses 789 932 +143 ☔ View full report in Codecov by Sentry. |
matt-bernstein
force-pushed
the
fb-dia-953/stream-results
branch
from
5b4e508 to
d31318c
Compare
pyproject.toml
Outdated
| @@ -35,6 +35,7 @@ fastapi = "^0.104.1" | |||
| celery = {version = "^5.3.6", extras = ["redis"]} | |||
| uvicorn = "*" | |||
| pydantic-settings = "^2.2.1" | |||
| label-studio-sdk = "^0.0.32" | |||
There was a problem hiding this comment.
Where did we land on making this an optional dependency?
I know poetry doesn't directly support this, but imo it's probably better to document that this dep is necessary if you want to use the LSE result handler (and then we can include it in our deployment, but not force users to have it to use adala)
There was a problem hiding this comment.
from comment above:
# these are for the server
# they would be installed as `extras` if poetry supported version strings for extras, but it doesn't
# https://github.com/python-poetry/poetry/issues/834
# they also can't be installed as a `group`, because those are for dev dependencies only and could not be included if this package was pip-installed
Are we ok with everyone (including external users/contributors) needing to clone this repo in order to use the server in stead of pip installing adala? I could see it going either way.
There was a problem hiding this comment.
Agreed that the server dependencies are questionable here too, and our deps are in a questionable state no matte what, but this one at least seems to be stemming from a different issue:
- the server deps should only be required by users of the server. These be in their own section (or repo/package), and the fix for that would eventually be breaking the server out into it's own repo/package
- the LSE SDK dep should be an optional dep even for users of the server, and even if/when we break that into its own repo, we'll still want it to be separate. (Granted, we'd still want to use
extrasfrom poetry, which we won't be able to. But since it's currently only one additional dep to install for the LSE handler, I think it's preferable to just document that you need to havelabel-studio-sdkinstalled to use the handler than it is to force anyone who wants the server to havelabel-studio-sdkinstalled)
There was a problem hiding this comment.
Oh, I see what you mean - can do something like put the label_studio_sdk import inside this class, catch and surface the error from it not being available, and update our deployment setup to manually add the dep.
There was a problem hiding this comment.
done, not sure what needs to be done to install adala like this in deployment env:
poetry install --with label-studio
There was a problem hiding this comment.
Need to tell plate about it probably as well update our own dockerfile
There was a problem hiding this comment.
added to dockerfile
There was a problem hiding this comment.
@farioas we're adding the label studio SDK as an optional dep to Adala, is there anything that needs to be done manually here for our dev instance of adala to keep working? The dockerfile is already updated in this branch.
