-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Check that agent name is valid #4158
Conversation
That issue corresponds to 0.4. We better not make this change in 0.2 as it would certainly be breaking and is not necessary. |
Sorry now I see the matching issue you created. If this check is to be added then I think the regex should match that if the one shown in the error. As it is they aren’t quite the same. Whether the main library should depend on OpenAI’s definition of the name of an agent is another question. Buy given the general tight coupling in 0.2 it's not unreasonable |
Thank you @jackgerrits, I did a regex based check. |
Thinking more on this I am a little hesitant. The root cause of the original issue is name constraints openai has, however, this solution applies those constraints to all agents regardless of whether they use openai, another llm or no llm. So I don't want to break peoples use cases |
This is true @jackgerrits. If you think there is a point in making it openai specific let me know. Otherwise we can close this pr. |
I appreciate the work here and response. Given that for it to be OpenAI specific it would need to be checked within the model client and special cased to known OpenAI endpoints, at which point you know OpenAI will end up producing a similar error anyway. To be fair, it could be shown at configure time vs runtime but the gain is minimal. Also, this problem goes away completely in 0.4 since we are imposing naming restrictions on agents from the get go to avoid this sort of problem. I'm going to go ahead and close this and the issue as not-planned, but if you feel strongly on adding a configure-time check them please feel free to reopen them. Thanks for your engagement! |
Why are these changes needed?
Display a clear error when the agent name is not a valid according to OpenAI's requirements
Related issue number
Closes #4157
Checks