Skip to content
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

Remove assistant_funtions from RolePlaying #249

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

lightaime
Copy link
Member

Description

Remove assistant_funtions from RolePlaying to simplify the RolePlaying API and fix a bug: #248

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • Example (update in the folder of example)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

Copy link
Member

@HalberdOfPineapple HalberdOfPineapple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response. This looks good to me and I think it makes sense :) (but I guess the function_call can be removed during initialization of assistant_model_config in the example as it can direcly use the default value.)

@lightaime
Copy link
Member Author

Sorry for the late response. This looks good to me and I think it makes sense :) (but I guess the function_call can be removed during initialization of assistant_model_config in the example as it can direcly use the default value.)

Sorry that I do not get it completely. Can you explain in detail or comment on the code?

@HalberdOfPineapple
Copy link
Member

HalberdOfPineapple commented Aug 21, 2023

Sorry that I do not get it completely. Can you explain in detail or comment on the code?

Sorry for the confusion. It is simply writing the initialization as below. A very minimal adjustment.

function_list = [*MATH_FUNCS, *SEARCH_FUNCS]
assistant_model_config = FunctionCallingConfig.from_openai_function_list(function_list=function_list)

@dandansamax
Copy link
Collaborator

Sorry that I do not get it completely. Can you explain in detail or comment on the code?

Sorry for the confusion. It is simply writing the initialization as below. A very minimal adjustment.

function_list = [*MATH_FUNCS, *SEARCH_FUNCS]
assistant_model_config = FunctionCallingConfig.from_openai_function_list(function_list=function_list)

Sorry for interrupting.
I'm updating test for RolePlaying and I have a small question. How can I set other parameters in config if I use FunctionCallingConfig.from_openai_function_list?

Copy link
Member

@HalberdOfPineapple HalberdOfPineapple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for interrupting. I'm updating test for RolePlaying and I have a small question. How can I set other parameters in config if I use FunctionCallingConfig.from_openai_function_list?

Good point and I add an extra kwargs parameter in from_openai_function_list to allow extra modification on the orignal configuration. @lightaime @dandansamax Could you have a check on this?

@lightaime
Copy link
Member Author

Sorry for interrupting. I'm updating test for RolePlaying and I have a small question. How can I set other parameters in config if I use FunctionCallingConfig.from_openai_function_list?

Good point and I add an extra kwargs parameter in from_openai_function_list to allow extra modification on the orignal configuration. @lightaime @dandansamax Could you have a check on this?

Great. It looks good. Please feel free to merge it.

@dandansamax
Copy link
Collaborator

Sorry for interrupting. I'm updating test for RolePlaying and I have a small question. How can I set other parameters in config if I use FunctionCallingConfig.from_openai_function_list?

Good point and I add an extra kwargs parameter in from_openai_function_list to allow extra modification on the orignal configuration. @lightaime @dandansamax Could you have a check on this?

Very good. Thanks!

Copy link
Member

@HalberdOfPineapple HalberdOfPineapple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the agreements :)

@HalberdOfPineapple HalberdOfPineapple merged commit e1258d3 into master Aug 21, 2023
10 checks passed
@HalberdOfPineapple HalberdOfPineapple deleted the remove_assistant_functions branch August 21, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Modifies the API Example
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants