Skip to content
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

Merged
merged 11 commits into from
Mar 27, 2024
Merged

local models #233

merged 11 commits into from
Mar 27, 2024

Conversation

FelipeAdachi
Copy link
Contributor

Langkit downloads models from HF Hub, which will hit an error in network-restricted environments. Even though modules such as toxicity,themes, and input_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:

from langkit import LangKitConfig

local_config = LangKitConfig(toxicity_model_path="local-toxicity-model",
              transformer_name="local-sentence-transformers")

toxicity.init(config=local_config)
themes.init(config=local_config)
input_output.init(config=local_config)

provided the supported models were already downloaded and stored in local-toxicity-model and local-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

FelipeAdachi and others added 2 commits February 27, 2024 15:05
Co-authored-by: richard-rogers <93153899+richard-rogers@users.noreply.github.com>
Co-authored-by: richard-rogers <93153899+richard-rogers@users.noreply.github.com>
Copy link
Collaborator

@jamie256 jamie256 left a 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/utils.py Outdated Show resolved Hide resolved
langkit/utils.py Outdated Show resolved Hide resolved
Comment on lines 53 to 57
if _model_path is None:
raise ValueError("Must initialize model path before calling toxicity!")
_model.value
_toxicity_tokenizer.value
_toxicity_pipeline.value
Copy link
Collaborator

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?

Copy link
Contributor Author

@FelipeAdachi FelipeAdachi Mar 5, 2024

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

langkit/transformer.py Outdated Show resolved Hide resolved
felipe207 and others added 4 commits March 5, 2024 18:48
Copy link
Collaborator

@jamie256 jamie256 left a 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.

@FelipeAdachi FelipeAdachi merged commit a818af7 into main Mar 27, 2024
12 checks passed
@FelipeAdachi FelipeAdachi deleted the dev/felipe/local-models branch March 27, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants