Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: DIA-953: Stream results from adala inference server into LSE #75
feat: DIA-953: Stream results from adala inference server into LSE #75
Changes from 17 commits
d31318c
da620e0
dc07ac9
fd2e40e
73887a9
1f5ec71
c286862
42c19ee
a956784
e1c2748
466e299
fc6e6d8
58c686a
825ce54
4812cb1
af1d975
00505dc
2e02ebb
f205c7c
64e174a
4e37052
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from comment above:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
extras
from 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-sdk
installed to use the handler than it is to force anyone who wants the server to havelabel-studio-sdk
installed)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to tell plate about it probably as well update our own dockerfile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added to dockerfile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.