Add support for llm in spacy init config
#12820
Conversation
rmitsch
added
enhancement
|
As of now Alternatively we could also required the registry handle to be set explicitly: e. g. Opinions? |
| @@ -3,7 +3,7 @@ the docs and the init config command. It encodes various best practices and | |||
| can help generate the best possible configuration, given a user's requirements. #} | |||
| {%- set use_transformer = hardware != "cpu" and transformer_data -%} | |||
| {%- set transformer = transformer_data[optimize] if use_transformer else {} -%} | |||
| {%- set listener_components = ["tagger", "morphologizer", "parser", "ner", "textcat", "textcat_multilabel", "entity_linker", "span_finder", "spancat", "spancat_singlelabel", "trainable_lemmatizer"] -%} | |||
| {%- set listener_components = ["tagger", "morphologizer", "parser", "ner", "textcat", "textcat_multilabel", "entity_linker", "span_finder", "spancat", "spancat_singlelabel", "trainable_lemmatizer", "llm"] -%} | |||
There was a problem hiding this comment.
llm shouldn't be in this list of components with listeners, which will cause tok2vec or transformer to be added (depending on the rest of the template, of course).
There was a problem hiding this comment.
Makes sense. I vaguely remember that the LLM block was not output properly if "llm" was not in listener_components. I'll check.
| {% if "llm" in components -%} | ||
| [components.llm] | ||
| factory = "llm" | ||
|
|
||
| [components.llm.model] | ||
| @llm_models = "{{ llm_spec['model'] }}" | ||
|
|
||
| [components.llm.task] | ||
| @llm_tasks = "{{ llm_spec['task'] }}" | ||
| {% endif -%} | ||
|
|
There was a problem hiding this comment.
I think (depending on exactly how this gets designed in the end), it should be possible to only have this block once because it does not depend on the listener setup.
| @pytest.mark.parametrize("pipeline", [["llm"]]) | ||
| @pytest.mark.parametrize("llm_model", ["noop"]) | ||
| @pytest.mark.parametrize("llm_task", ["ner", "sentiment"]) | ||
| def test_init_config_llm(pipeline, llm_model, llm_task): |
There was a problem hiding this comment.
I think this test should probably be in spacy-llm unless it's installed by default (similar to spacy-transformers, where I'm not sure this is tested at all, but where it could be tested).
There was a problem hiding this comment.
IMO we should consider installing spacy-llm by default going forward. At some point we want to move it to the core codebase, but for now, having it installed by default + kept as separate repo has the advantages that we can still release updates more quickly with spacy-llm while not troubling users with an additional install command. The requirements are minimal.
There was a problem hiding this comment.
This is currently not working for most OS HF models, as they don't have a default name like e.g. ""spacy.GPT-3-5.v1" does.
For instance, spacy init config myconfig.cfg -p "llm" --llm.model "Falcon" --llm.task 'ner'
will fail with
✘ Config validation error
llm.model -> name field required
{'@llm_models': 'spacy.Falcon.v1'}
| valid_values.add(reg_name) | ||
| if reg_name.lower() == user_value: | ||
| spec["matched_reg_handle"] = reg_handle | ||
| break |
There was a problem hiding this comment.
If there are 2 versions of a model/task, is this code guaranteed to give you the latest version?
There was a problem hiding this comment.
It doesn't - I just checked - it produces spacy.NER.v1 instead of spacy.NER.v2 when given "ner"
There was a problem hiding this comment.
No, it just grabs the first occurence as of now. I left that as TBD since it wasn't clear how we want to go forward.
Description
Add support for
llmcomponent. inspacy init configTypes of change
Enhancement.
Checklist