Python a2ui_agent: Refactor a2ui_agent to depend on a2ui_core#1582
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the a2ui_core Python library, which implements core state management, rendering, message processing, and schema validation for the A2UI v0.9+ specification. Key feedback highlights several critical issues and improvement opportunities: a NameError in model_catalog.py due to an undefined variable, incomplete TR35 date token formatting in function_impls.py, redundant subscriptions for TemplateChildList in generic_binder.py, an inefficient list expansion loop in data_model.py that could trigger OOM errors, silent acceptance of unclosed string literals in expression_parser.py, and a potential AttributeError in data_context.py when handling malformed action events.
nan-yu
force-pushed
the
re-arch-9-agent
branch
2 times, most recently
from
dee8acb to
2139170
Compare
jacobsimionato
left a comment
jacobsimionato
left a comment
There was a problem hiding this comment.
It looks like this PR goes a long way to removing logic from the existing agent SDK and having it depend on core instead which is great.
Do you plan to keep tweaking it after this? I think it might be possible to do some more cleanups and deduplication! But this seems like a big step towards the architecture we want, and it's nice that it seems like all the existing APIs are preserved.
| custom_cuttable_keys: Optional[frozenset[str]] = None | ||
|
|
||
| @classmethod | ||
| def from_path( |
There was a problem hiding this comment.
I wonder if this class should be replaced with the new Catalog object from Core, or a function that returns a Catalog object (to defer async file system loads if necessary). That way, Catalog is the once source of truth, and developers can create them from a JSON file, or programmatically.
I think a workflow that is critical is to define a Catalog programmatically from a set of components and functions, and then use the Schema manager etc to create a prompt from it and parse the results. Right now, it seems like we're using the new Catalog abstraction for validation, but it's not yet possible to start with a Catalog object and then generate prompts from it. Ideally, the core Catalog representation should be the source of truth for the whole app.
This can be done in a future PR!
There was a problem hiding this comment.
Sure, the catalog class can be further refactored to depend more on the Core catalog. I'll follow up.
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class A2uiCatalog: |
There was a problem hiding this comment.
Maybe we can rename this to InferenceCatalog or something at some point? And I think it should be instantiated from a Catalog!
There was a problem hiding this comment.
The renaming will be a breaking change. I'll wait until I'm back to start the migration ball.
nan-yu
force-pushed
the
re-arch-9-agent
branch
2 times, most recently
from
687e251 to
bd3d661
Compare
…resh dependencies across samples and tools
…ent type references
|
Looks like this broke the e2e tests. #1648. The evals failure is unrelated, and already fixed. I'll take a look at why. |
3 participants
Description
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. For larger changes, raising an issue first helps reduce redundant work.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.