aiconfig
aiconfig copied to clipboard
Check model and model parser ids strings for spaces, strip them
Check model and model parser ids strings for spaces, strip them
Tanya noticed a bug when we accidentally added "field " with a space instead of "field", so trimming the value.
I originally tried to fix this in the editor config itself in SettingsPropertyRenderer
, but that was complicated since it meant that this would reset the string value state for the string, and you couldn't keep typing empty spaces at the begininng or end of strings otherwise it would keep trimming them. Therefore, I decided to make this change in all places where we set the model names or fields for main SDK. This may seem a bit controversial (ex: you now can't have two separate models called " my_model " vs. "my_model" but I feel this is fine
Note: I changed both the getter and setter methods, so this will be a breaking change for existing AIConfigs that have leading or trailing whitespace in the (model)/(model parser) names
Test Plan
aiconfig_path=./cookbooks/Gradio/huggingface.aiconfig.json
parsers_path=./cookbooks/Gradio/aiconfig_model_registry.py
aiconfig edit --aiconfig-path=$aiconfig_path --server-port=8080 --server-mode=debug_servers --parsers-module-path=$parsers_path
It now works with leading or trailing whitespace
https://github.com/lastmile-ai/aiconfig/assets/151060367/116b4ef4-b1f9-4a15-9d07-e1fb9d995245
Stack created with Sapling. Best reviewed with ReviewStack.
- -> #1114
- #1112
I originally tried to fix this in the editor config itself in SettingsPropertyRenderer, but that was complicated since it meant that this would reset the string value state for the string, and you couldn't keep typing empty spaces at the begininng or end of strings otherwise it would keep trimming them.
Couldn't it just be done in onUpdatePromptModel
in AIConfigEditor
? Just trim the newModel before dispatch/request
This may seem a bit controversial (ex: you now can't have two separate models called " my_model " vs. "my_model" but I feel this is fine
I do agree, I don't think leading/trailing whitespace should be part of the model name since that's asking for trouble in any other place that trims things.
Couldn't it just be done in
onUpdatePromptModel
inAIConfigEditor
? Just trim the newModel before dispatch/request
It's a bit more complicated than this, because we get the model_name from the prompt when calling config.run()
in https://github.com/lastmile-ai/aiconfig/blob/c571904edfafd878e60c81962a85b3f9f7de5125/python/src/aiconfig/Config.py#L322-L333
~~So in order to prevent breaking changes to existing AIConfigs which may have spaces already defined in that config and not trimmed, we need to continue using those ones~~.
Ok nm, I realize that since we already strip the model name in register_model_parser()
, which is what we call every time we do config.create()
or config.load()
then we're fine.
However I still feel it's good to save it in the SDK so that this is centralized and not reliant on the editor client logic. Ex: if someone manually calls config.update_model(" my_model ", None, "prompt_name")
it will still work
Is there some minimum that we can do without needing to do it everywhere?
I think that for future nit, we can call sanitize_model_name
before every centralized and public SDK method involving model_name. Created task for it in https://github.com/lastmile-ai/aiconfig/issues/1126