Added support for AG-UI Multi-modal Messages #3715
Conversation
| @bojan2501 Can you have a look at the missing test coverage please? |
| I think this is it. |
| @DouweM To be honest I do not understand why the checks are failing now. |
| return builder.messages | ||
| | ||
| @classmethod | ||
| def load_binary_part(cls, part: BinaryInputContent) -> BinaryContent | ImageUrl | VideoUrl | AudioUrl | DocumentUrl: |
There was a problem hiding this comment.
These methods really shouldn't be part of the public interface, so please make them private, either has class methods, or if that doesn't work, as private functions in the module, outside of the class
tests/test_ag_ui.py Outdated
| ) | ||
| events = await run_and_collect_events(agent, run_input) | ||
| | ||
| assert events == simple_result() |
There was a problem hiding this comment.
Let's drop all of the tests above because they're not verifying that the binary content actually made it through. I'd rather expand the test_messages test
tests/test_ag_ui.py Outdated
| assert not loaded_messages | ||
| | ||
| | ||
| def test_load_binary_part() -> None: |
There was a problem hiding this comment.
I'd rather test this at the load_messages level so we can verify these end up in the correct Pydantic AI message history, not just individual parts
tests/test_ag_ui.py Outdated
| AGUIAdapter.load_binary_part(BinaryInputContent(id='some_id', mime_type='text/plain')) | ||
| | ||
| | ||
| def test_add_assistant_tool_parts() -> None: |
There was a problem hiding this comment.
We shouldn't need to test these helper methods directly, as all of these features have already been tested through the existing tests, except for the new binary content. So I'd expect for the only new tests to be related to those binary parts.
If these tests were necessary because of the refactoring of the original massive method into separate ones, I'd prefer to revert that and have a massive method again, which was 100% test-covered.
| @DouweM
bab3876#diff-84321598744d84dbee2318e634c74c9aae39a1c253f1c4bd17ebf9ef2f807b11 I see now that something went wrong, and there is a conflict. Should I cancel this pull request and create fresh one? Could be much more easy for review. |
…sages. Updated existing tests to test new content. Bumped ag-ui version.
DouweM force-pushed the feature/3558-Add-Support-For-Multi-Modal-Messages-AG-UI branch from bab3876 to 6c206fc Compare | @bojan2501 I've fixed this branch PR as follows: git co feature/3558-Add-Support-For-Multi-Modal-Messages-AG-UI # your branch git fetch origin # my origin remote points at the canonical repo git reset --hard origin/main # reset to match main branch, removing all history on the branch git cherry-pick bab3876a9315ae058e6d3b9e197425afdec38e1c # apply just the commit with the intended changes git push -f # force push to overwrite branch historyLet's see what CI says! |
| @bojan2501 What was the rationale for adding an exception to There is nothing in the AG-UI spec which requires such: you've now required clients to do this filtering, which might or might not be desirable. For instance, agents implemented as graphs with human-in-the-loop or client-side tool calling might use the |
| Hi @tseaver, My intention was to add support for Multi Modal messages. I could review this again. |
| @bojan2501 Thanks for the reply. If the translation to |
| @tseaver Feel free to submit a new PR to remove the error! |
3 participants
Addresses 3558
Refactored code for better readability.
Bumped version of ag-ui libs.