Add function calling support using bolt-fc-1b #35
Conversation
adilhafeez
changed the title
function_resolver/app/template.py
Outdated
| elements += self.handler._format_system(tools) | ||
|
|
||
| if message.role == "user": | ||
| elements += self.handler._format_user(content=message.content) |
There was a problem hiding this comment.
use dict or a better way to handle this
function_resolver/requirements.txt
Outdated
function_resolver/test/test.sh
Outdated
There was a problem hiding this comment.
we should organize using makefile in the future
function_resolver/app/template.py
Outdated
| Converts elements to token ids. | ||
| """ | ||
| token_ids = [] | ||
| for elem in elements: |
There was a problem hiding this comment.
this loop would be slow, any chance we can vectorize this?
|
@adilhafeez Thanks for making this PR! Here are some changes I think you need to make:
|
| pub score: f64, | ||
| pub struct ToolParameters { | ||
| #[serde(rename = "type")] | ||
| pub parameters_type: String, |
There was a problem hiding this comment.
why not enum? also why not simply name it type?
There was a problem hiding this comment.
it should be enum, will update
| pub parameter_type: Option<String>, | ||
| pub description: String, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub required: Option<bool>, |
There was a problem hiding this comment.
This is to say that required parameter is optional to enable following use case. In this spec, when required is missing then parameter is optional. For example city is required and days, units are optional parameters.
parameters:
- name: city
required: true
description: The city for which the weather forecast is requested.
- name: days
description: The number of days for which the weather forecast is requested.
- name: units
description: The units in which the weather forecast is requested.
There was a problem hiding this comment.
Interesting API choice, instead of using required: false
public-types/src/common_types.rs
Outdated
| pub model: String, | ||
| pub created_at: String, |
There was a problem hiding this comment.
updated to use chrono::DateTime
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[serde(untagged)] | ||
| pub enum IntOrString { |
There was a problem hiding this comment.
IMO it is more descriptive to name the int and the string by names that tell us why someone would use a string or an int in this field.
| } | ||
|
|
||
| pub mod open_ai { | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| use super::ToolsDefinition; | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct ChatCompletions { | ||
| #[serde(default)] | ||
| pub model: String, |
There was a problem hiding this comment.
What is the difference between the model here and in the message?
envoyfilter/src/stream_context.rs
Outdated
| debug!("function_resolver response str: {:?}", body_str); | ||
|
|
||
| let mut resp = serde_json::from_str::<FunctionCallingModelResponse>(&body_str).unwrap(); | ||
| resp.resolver_name = Some(callout_context.prompt_target.as_ref().unwrap().name.clone()); |
There was a problem hiding this comment.
A lot of unwrap re-introduction all over the place in this PR. Is this the best error handling we can do?
| info!("possibly some required parameters are missing, send back the response"); | ||
| let resp_str = serde_json::to_string(&resp).unwrap(); | ||
| self.send_http_response( | ||
| StatusCode::OK.as_u16().into(), |
There was a problem hiding this comment.
So why would this be 200, if the response was invalid?
There was a problem hiding this comment.
This part needs to be rewritten - will update this code block.
There was a problem hiding this comment.
rewrote this part, its better now
envoyfilter/src/stream_context.rs
Outdated
| return; | ||
| } | ||
| }; | ||
| let tool_call_details = _tool_call_details.unwrap(); |
There was a problem hiding this comment.
All of this block of code seems to be like this due to all the print line debugging, did you mean to clean this up?
| // add original user prompt | ||
| messages.push({ | ||
| Message { | ||
| role: USER_ROLE.to_string(), |
There was a problem hiding this comment.
should role and model be enums?
There was a problem hiding this comment.
We don't want to restrict role and model to be limited to fixed values. New models keep coming in. And to support new model it would require change in spec.
For Role I am still thinking wether we should use enum or not. I am leaning toward using enum and list all possible roles in spec that are used by API based LLMs that we support in open-message-format.
There was a problem hiding this comment.
With all the inline requests to local servers I have been thinking that this code would benefit from some sort of strongly typed state machine pattern inspired by a common rust pattern, examples:
There was a problem hiding this comment.
@junr03 I've been thinking about it too. The orchestration inside filter can be viewed and programmed as state machine. And if done right, it can become much easier to visualize and reason.
|
Also -- lint and test are still not passing. |
This code change adds bolt-fc-1b to bolt gateway. There are several changes that went in to support bolt-fc-1b. Here is a summary of key changes.
config-generator, its task is to take envoy.template.yaml, katanemo-config and generate final envoy.yaml that can be used by envoy to bootstrap