aiconfig icon indicating copy to clipboard operation
aiconfig copied to clipboard

Check model and model parser ids strings for spaces, strip them

Open rossdanlm opened this issue 1 year ago • 3 comments

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

rossdanlm avatar Feb 02 '24 09:02 rossdanlm

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.

rholinshead avatar Feb 02 '24 20:02 rholinshead

Couldn't it just be done in onUpdatePromptModel in AIConfigEditor? 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

rossdanlm avatar Feb 03 '24 00:02 rossdanlm

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

rossdanlm avatar Feb 03 '24 00:02 rossdanlm