-
Notifications
You must be signed in to change notification settings - Fork 53
overhaul tools for model calls #88
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
Conversation
…acting tools from list of actions
|
This all seems reasonable and the code looks reasonable. I need to think about this from a design perspective without an over-tired mind. But I probably won't get a chance to do that until next week, and Hendrik is also AFK. Is there anything urgent here? Is there any work blocked by this? |
|
This (or some other approach) is necessary for sampling strategies with tools (namely transform). Not urgent / I have other things to work on until you can re-review. |
|
I think my main point of issue with this PR would be the additional complexity it potentially adds to contexts, but in reality the new function to get the available actions will almost always just be the same as the generation context. Additionally, as long as we are clear about how tools are made available in our documentation, I think this approach is actually more intuitive. You could imagine adding an object to a context and then later on chatting or writing an instruction that requires using a tool from that object. |
|
I like the code. The only danger I see is that giving ModelOptions.TOOLS lowest priority in the overwriting hierarchy might be right for session-bound model options but I would assume that ModelOptions.TOOLS coming in as direct parameter of an |
|
This is not necessarily a valid assumption but my hope was that ModelOptions.TOOLS would be more generic functions like (get_weather, web_search, etc...) and then the specific tools would be the ones coming from Components. It seems like there should be very little overlap there (but let me see if I can get decent warning logging happening here as well). My reasoning for having the Component based tools take priority is focused on |
|
Yeah, I can see how this would be confusing if we go one way or the other. It's a bit of an annoying crux. It it clear which one should have priority? Would it be surprising in one case or another? Maybe we should have an Absent any strong objections, maybe we just go ahead with this approach and stay open to either changing priority or requiring explicit indication of priority (via |
Yeah, that's the problem. I'm not quite sure what the "expected" behavior is / how people will feel about it. I like the override flag, but am okay with waiting to see if people request something like that. I've also opened #95 for allowing tool renaming. If we support that, we can also avoid some conflicts by automatically renaming tools from components to be @HendrikStrobelt, are you okay with merging this as is? |
Overhauled how tools are made available to the model generation request. Instead of only relying on the tools for the current action being generated from, contexts can now provide a list of actions to get tools from. By default, this list is the same list of actions added to the conversation for messages.
This change is required to support more complicated sampling strategies. When allowing for mulit-turn resolution of prompts, it might be necessary to have several user messages with the model.
For example, if I asked the model to add a row to a table based on the current weather, it would first have to do a weather lookup and then call the table function. However, after the weather lookup message, the table function would no longer be an available tool since it's no longer the most recent action (the answer to the weather lookup is). With this change, the tool will still be available to the model.
Additional changes:
All tests passed.