-
Notifications
You must be signed in to change notification settings - Fork 270
feat: render raw curl for aws-bedrock #2319
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
feat: render raw curl for aws-bedrock #2319
Conversation
@ethanlijin is attempting to deploy a commit to the Boundary Team on Vercel. A member of the Team first needs to authorize it. |
// region/profile hints | ||
if let Some(region) = &self.properties.region { | ||
cmd.push_str(&format!(" --region '{}'", escape(Cow::Borrowed(region)))); | ||
} | ||
if let Some(profile) = &self.properties.profile { | ||
cmd.push_str(&format!(" --profile '{}'", escape(Cow::Borrowed(profile)))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should be formatted to prefix the command e.g. AWS_REGION=us-east2 AWS_PROFILE=boundaryml-dev aws bedrock-runtime converse
// --model-id | ||
cmd.push_str(&format!( | ||
" --model-id '{}'", | ||
escape(Cow::Borrowed(&self.properties.model)) | ||
)); | ||
|
||
// --messages | ||
let messages_str = serde_json::to_string(&messages_json)?; | ||
let messages_escaped = escape(Cow::Borrowed(&messages_str)); | ||
cmd.push_str(&format!(" --messages {}", messages_escaped)); | ||
|
||
// --system (optional) | ||
if let Some(blocks) = system_blocks { | ||
let system_str = serde_json::to_string(&blocks)?; | ||
let system_escaped = escape(Cow::Borrowed(&system_str)); | ||
cmd.push_str(&format!(" --system {}", system_escaped)); | ||
} | ||
|
||
// --inference-config (optional) | ||
if let Some(cfg) = &self.properties.inference_config { | ||
let mut map = serde_json::Map::new(); | ||
if let Some(v) = cfg.max_tokens { | ||
map.insert("maxTokens".into(), json!(v)); | ||
} | ||
if let Some(v) = cfg.temperature { | ||
map.insert("temperature".into(), json!(v)); | ||
} | ||
if let Some(v) = cfg.top_p { | ||
map.insert("topP".into(), json!(v)); | ||
} | ||
if let Some(v) = cfg.stop_sequences.as_ref() { | ||
map.insert("stopSequences".into(), json!(v)); | ||
} | ||
if !map.is_empty() { | ||
let cfg_str = serde_json::to_string(&map)?; | ||
let cfg_escaped = escape(Cow::Borrowed(&cfg_str)); | ||
cmd.push_str(&format!(" --inference-config {}", cfg_escaped)); | ||
} | ||
} | ||
|
||
// --additional-model-request-fields (optional) | ||
if !self.properties.additional_model_request_fields.is_empty() { | ||
let addl = serde_json::to_value(&self.properties.additional_model_request_fields)?; | ||
let addl_str = serde_json::to_string(&addl)?; | ||
let addl_escaped = escape(Cow::Borrowed(&addl_str)); | ||
cmd.push_str(&format!( | ||
" --additional-model-request-fields {}", | ||
addl_escaped | ||
)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn image_format_from_mime(mime: &str) -> String { | ||
match mime.strip_prefix("image/") { | ||
Some(s) => s.to_string(), | ||
None => mime.to_string(), | ||
} | ||
} | ||
|
||
fn video_format_from_mime(mime: &str) -> Result<&'static str> { | ||
Ok(match mime { | ||
"video/mp4" => "mp4", | ||
"video/mpeg" => "mpeg", | ||
"video/mov" => "mov", | ||
"video/x-flv" => "flv", | ||
"video/mkv" => "mkv", | ||
"video/webm" => "webm", | ||
_ => anyhow::bail!( | ||
"AWS Bedrock video format not supported: {}. Supported formats: mp4, mpeg, mov, flv, mkv, webm", | ||
mime | ||
), | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't do this, just always strip the mime-type prefix (aka before the /
) and then let the aws api return errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 4a10e4e in 1 minute and 49 seconds. Click for details.
- Reviewed
229
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-runtime/src/internal/llm_client/primitive/aws/aws_client.rs:586
- Draft comment:
The helper function 'strip_mime_prefix' is quite naive. Consider a more robust MIME parsing approach if inputs might be nonstandard. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. engine/baml-runtime/src/internal/llm_client/primitive/aws/aws_client.rs:723
- Draft comment:
Pretty-printing JSON for the CLI argument may introduce newlines that can affect shell command behavior. Consider using a compact JSON string if unexpected formatting issues arise. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code is already using shell_escape::escape() which properly escapes special characters including newlines for shell usage. The shell_escape crate is specifically designed for this use case. Pretty-printing the JSON won't cause issues since any newlines will be properly escaped. The comment seems overly cautious and unnecessary given the existing safety measures. Could there be edge cases where shell_escape doesn't handle newlines correctly? Should we err on the side of caution with shell commands? The shell_escape crate is well-tested and specifically designed for this purpose. Pretty-printed JSON with proper escaping is actually more readable and debuggable for users seeing the curl command. The comment should be deleted. The code is already safely handling shell escaping via the shell_escape crate, and pretty-printed JSON provides better readability.
3. engine/baml-runtime/src/internal/llm_client/primitive/aws/aws_client.rs:647
- Draft comment:
There is duplicated logic converting ChatMessagePart variants to CLI JSON blocks. Consider refactoring this into a shared helper to reduce code duplication. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_F7zU5zjPnq6R1vkf
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
||
// TODO(sam): this is fucked up. The SDK actually hides all the serializers inside the crate and doesn't let the user access them. | ||
if let Some((first, remainder_slice)) = chat_slice.split_first() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting the system block conversion logic into a helper. The same pattern for processing ChatMessagePart (for system messages) appears later, and merging them could improve maintainability.
…to --cli-input-json; always strip MIME-type prefix for media in raw curl
…ON and prefixes env (AWS_REGION/AWS_PROFILE); stop validating mime in raw CLI (strip subtype only)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with two last things that need fixing
// Always strip MIME prefix and let AWS validate/err. | ||
let format = bedrock::types::VideoFormat::from(strip_mime_prefix( | ||
&media.mime_type_as_ok()?, | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aaggghhh. i was wrong, we should put back the manual mime type matching - this will not delegate to the bedrock api server but to the bedrock sdk and will actually result in worse error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's undo the changes to this file - i don't really understand why we're changing this
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Split out only the raw-curl implementation from previous branch @sxlijin <!-- ELLIPSIS_HIDDEN --> ---- > [!IMPORTANT] > Adds `render_raw_curl` to `AwsClient` for generating AWS Bedrock API curl commands, handling text and media content, and formatting with environment variables. > > - **Behavior**: > - Adds `render_raw_curl` method to `AwsClient` in `aws_client.rs` to generate raw curl commands for AWS Bedrock API requests. > - Handles text and media content, constructing JSON payloads for `system` and `messages`. > - Formats curl command with environment variables for `AWS_REGION` and `AWS_PROFILE`. > - **Functions**: > - Introduces `strip_mime_prefix` and `to_cli_content_block` helper functions for media handling. > - Uses `shell_escape::escape` for safe command-line argument formatting. > - **Error Handling**: > - Logs errors for unsupported media types and incomplete configurations. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=BoundaryML%2Fbaml&utm_source=github&utm_medium=referral)<sup> for 4a10e4e. You can [customize](https://app.ellipsis.dev/BoundaryML/settings/summaries) this summary. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN --> --------- Co-authored-by: Sam Lijin <sam@boundaryml.com>
Split out only the raw-curl implementation from previous branch
@sxlijin
Important
Adds
render_raw_curl
toAwsClient
for generating AWS Bedrock API curl commands, handling text and media content, and formatting with environment variables.render_raw_curl
method toAwsClient
inaws_client.rs
to generate raw curl commands for AWS Bedrock API requests.system
andmessages
.AWS_REGION
andAWS_PROFILE
.strip_mime_prefix
andto_cli_content_block
helper functions for media handling.shell_escape::escape
for safe command-line argument formatting.This description was created by
for 4a10e4e. You can customize this summary. It will automatically update as commits are pushed.