Read embeddings config during bootstrap/load and instance creation #387
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
markstur
requested review from
alex-jw-brooks,
gkumbhat,
evaline-ju,
gabe-l-hart,
tharapalanivel and
Ssukriti
as code owners
When CONFIG_FILES setting is not used, the caikit-nlp config values are not read from environment variables unless caikit has already imported caikit-nlp. The code was loading the embedding config too soon in this case so the env vars were ignored. This commit reads the config during bootstrap and load for settings that are needed at bootstrap/load time and also reads config during module init for runtime settings. In addition: * bootstrap() kwargs can be used (e.g. for trust_remote_code param) * load() was made to work with model_path as ModuleConfig (because str is deprecated and spammy) * utils/env_val_to_int was removed because the caikit config handles this well * utils/env_val_to_bool was NOT YET removed because the caikit config does not handle this quite as well Signed-off-by: Mark Sturdevant <[email protected]>
markstur
force-pushed
the
embedding_get_config
branch
from
91d83be to
272eeb1
Compare
evaline-ju
reviewed
Sep 4, 2024
| assert 1 == conf.get("true", expected_default) | ||
| assert 1 == int(conf.get("true", expected_default)) | ||
|
|
||
| assert "oh-oh" == conf.get("non_int", 123) # default not used (got "uh-oh") |
There was a problem hiding this comment.
nit: oh-oh with the given example instead of uh-oh in comments?
There was a problem hiding this comment.
clarified with "non int str"
ModuleConfig should always have a model_path, but this check will make the error better if this gets called with an empty ModuleConfig. Signed-off-by: Mark Sturdevant <[email protected]>
Signed-off-by: Mark Sturdevant <[email protected]>
evaline-ju
approved these changes
Sep 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When CONFIG_FILES setting is not used, the caikit-nlp config values are not read from environment variables unless caikit has already imported caikit-nlp. The code was loading the embedding config too soon in this case so the env vars were ignored.
This commit reads the config during bootstrap and load for settings that are needed at bootstrap/load time and also reads config during module init for runtime settings.
In addition: