-
Notifications
You must be signed in to change notification settings - Fork 14.1k
remove flag for image preparation #29429
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1371,17 +1371,11 @@ impl Session { | |
| } = self | ||
| .reconstruct_history_from_rollout(turn_context, rollout_items) | ||
| .await; | ||
| if turn_context | ||
| .config | ||
| .features | ||
| .enabled(Feature::ResizeAllImages) | ||
| { | ||
| // Keep the recorded rollout unchanged. Prepare its reconstructed history before | ||
| // installing it, so legacy images are processed once for this resume or fork and | ||
| // will be processed again if the rollout is reconstructed in a future session. | ||
| // This meets image resizing requirements without modifying persisted rollouts. | ||
| prepare_response_items(&mut history); | ||
| } | ||
| // Keep the recorded rollout unchanged. Prepare its reconstructed history before | ||
| // installing it, so legacy images are processed once for this resume or fork and | ||
| // will be processed again if the rollout is reconstructed in a future session. | ||
| // This meets image resizing requirements without modifying persisted rollouts. | ||
| prepare_response_items(&mut history); | ||
| { | ||
| let mut state = self.state.lock().await; | ||
| state.replace_history(history, reference_context_item); | ||
|
|
@@ -2688,13 +2682,7 @@ impl Session { | |
| items: &'a [ResponseItem], | ||
| ) -> Cow<'a, [ResponseItem]> { | ||
| let mut items = Cow::Borrowed(items); | ||
| if turn_context | ||
| .config | ||
| .features | ||
| .enabled(Feature::ResizeAllImages) | ||
| { | ||
| prepare_response_items(items.to_mut()); | ||
| } | ||
| prepare_response_items(items.to_mut()); | ||
| if turn_context.config.features.enabled(Feature::ItemIds) { | ||
| Self::assign_missing_response_item_ids(items) | ||
| } else { | ||
|
|
@@ -2733,23 +2721,10 @@ impl Session { | |
| items | ||
| } | ||
|
|
||
| pub(crate) fn response_item_from_user_input( | ||
| &self, | ||
| turn_context: &TurnContext, | ||
| input: Vec<UserInput>, | ||
| ) -> ResponseItem { | ||
| let local_image_preparation = if turn_context | ||
| .config | ||
| .features | ||
| .enabled(Feature::ResizeAllImages) | ||
| { | ||
| LocalImagePreparation::Defer | ||
| } else { | ||
| LocalImagePreparation::Process | ||
| }; | ||
| pub(crate) fn response_item_from_user_input(&self, input: Vec<UserInput>) -> ResponseItem { | ||
| ResponseItem::from(ResponseInputItem::from_user_input( | ||
| input, | ||
| local_image_preparation, | ||
| LocalImagePreparation::Defer, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a user accidentally attaches a large non-image or oversized local file, this now always takes the Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not an issue |
||
| )) | ||
| } | ||
|
|
||
|
|
@@ -3615,7 +3590,7 @@ impl Session { | |
| // Persist the user message to history, but emit the turn item from `UserInput` so | ||
| // UI-only `text_elements` are preserved. `ResponseItem::Message` does not carry | ||
| // those spans, and `record_response_item_and_emit_turn_item` would drop them. | ||
| let response_item = self.response_item_from_user_input(turn_context, input.to_vec()); | ||
| let response_item = self.response_item_from_user_input(input.to_vec()); | ||
| self.record_conversation_items(turn_context, std::slice::from_ref(&response_item)) | ||
| .await; | ||
| let mut user_message_item = UserMessageItem::new(input); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| use codex_features::Feature; | ||
| use codex_protocol::items::ImageViewItem; | ||
| use codex_protocol::items::TurnItem; | ||
| use codex_protocol::models::DEFAULT_IMAGE_DETAIL; | ||
|
|
@@ -8,9 +7,7 @@ use codex_protocol::models::FunctionCallOutputPayload; | |
| use codex_protocol::models::ImageDetail; | ||
| use codex_protocol::models::ResponseInputItem; | ||
| use codex_protocol::openai_models::InputModality; | ||
| use codex_utils_image::PromptImageMode; | ||
| use codex_utils_image::data_url_from_bytes; | ||
| use codex_utils_image::load_for_prompt_bytes; | ||
| use serde::Deserialize; | ||
|
|
||
| use crate::function_tool::FunctionCallError; | ||
|
|
@@ -195,24 +192,8 @@ impl ViewImageHandler { | |
| DEFAULT_IMAGE_DETAIL | ||
| }; | ||
|
|
||
| let image_url = if turn.config.features.enabled(Feature::ResizeAllImages) { | ||
| // The history insertion path owns image decoding and resizing when this is enabled. | ||
| data_url_from_bytes("application/octet-stream", &file_bytes) | ||
| } else { | ||
| let image_mode = if use_original_detail { | ||
| PromptImageMode::Original | ||
| } else { | ||
| PromptImageMode::ResizeToFit | ||
| }; | ||
| load_for_prompt_bytes(abs_path.as_path(), file_bytes, image_mode) | ||
| .map_err(|error| { | ||
| FunctionCallError::RespondToModel(format!( | ||
| "unable to process image at `{}`: {error}", | ||
| abs_path.display() | ||
| )) | ||
| })? | ||
| .into_data_url() | ||
| }; | ||
| // The history insertion path owns image decoding and resizing. | ||
| let image_url = data_url_from_bytes("application/octet-stream", &file_bytes); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a serious issue |
||
|
|
||
| let item = TurnItem::ImageView(ImageViewItem { | ||
| id: call_id, | ||
|
|
@@ -420,9 +401,6 @@ mod tests { | |
| }) | ||
| .await; | ||
|
|
||
| let Err(FunctionCallError::RespondToModel(message)) = result else { | ||
| panic!("expected image processing error"); | ||
| }; | ||
| assert!(message.contains("unable to process image"), "{message}"); | ||
| result.expect("explicit high detail should be accepted"); | ||
| } | ||
| } | ||
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.
When code mode emits a data-URL image with
detail: "low"(which the code-mode parser still accepts and maps toImageDetail::Low), this unconditional preparation path now runs before the tool output reaches the next model request.prepare_imagetreatsImageDetail::Lowas unsupported and replaces the image with a text placeholder, so previously valid low-detail code-mode image outputs silently stop being images. Please either stop acceptinglowin code mode or normalize it to a supported detail before callingprepare_response_items.Useful? React with 👍 / 👎.
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.
expected