-
Notifications
You must be signed in to change notification settings - Fork 69
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
local models #233
local models #233
Conversation
Co-authored-by: richard-rogers <93153899+richard-rogers@users.noreply.github.com>
Co-authored-by: richard-rogers <93153899+richard-rogers@users.noreply.github.com>
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.
Left some preliminary comments.
langkit/toxicity.py
Outdated
if _model_path is None: | ||
raise ValueError("Must initialize model path before calling toxicity!") | ||
_model.value | ||
_toxicity_tokenizer.value | ||
_toxicity_pipeline.value |
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.
Curios if this is noticeably slower or not?
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.
I did a very simple test, iterating on 5k samples (single row extraction), and init_model()
takes, on average 0.0048 ms (4.8e-6 seconds) - ignoring the first call, which will actually load the model artifacts
update: on recent changes, the lazy initialization is now done for each row, even if it's a batch - it was changed to concile with the detoxify addition in another PR. If the added latency is prohibitive, we need to rethink the design proposed in this PR
# Conflicts: # langkit/docs/modules.md # langkit/toxicity.py
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.
LGTM!
Let's file an issue for local model support on other toxicity models.
Also consider a post_init() or other way to trigger initialization outside of an actual request. We could document that you can call predict on these models as a first time call that should trigger any downloads or initialization.
Langkit downloads models from HF Hub, which will hit an error in network-restricted environments. Even though modules such as
toxicity
,themes
, andinput_output
allow passing the path of local model, the auto-initialization on import will hit an error before we're able to pass the local path in the config.To deal with this, this change lazy initializes the models so it will fetch them when it's actually needed, when the udf is called. When using local model, one can do as below:
provided the supported models were already downloaded and stored in
local-toxicity-model
andlocal-sentence-transformers
. For reference, these are the HF models that are currently being used in Langkit:martin-ha/toxic-comment-model
sentence-transformers/all-MiniLM-L6-v2