refactor: introduce alias for component cblock modeloutputthunk union - #1458
refactor: introduce alias for component cblock modeloutputthunk union#1458AngeloDanducci wants to merge 2 commits into
Conversation
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
|
NodeData is too vague and familiar from other uses. In this case vague might be good, but it looks very borrowed from other things. Happy to defer to others, but if I'm going to critique then I should offer alternatives: Content Staying with "vague is good", I think Content will read nicely but is also overused (conflicty with other use of ".content"). I think ContentItem reads ok and ContentPiece seems to work great (but it just looks weird). Looking at the code ContentPart is maybe too specific? Otherwise I like that parts() returns ContentPart. Both Piece and Part unfortunately don't sound like things that could be stand-alone complete things. If nobody comes up with something that is better than NodeData. It certainly works. Vague is better than misleading! Maybe my suggestions will at least inspire a better choice. |
markstur
left a comment
markstur
left a comment
There was a problem hiding this comment.
Naming and "other issue" are not blockers, but unless I'm missing something these tests updates would be good to address before merge...
Tests have old parts() return type that should be updated to list[NodeData] as well:
Changes Needed in Missed Test Files
- test/core/test_component_typing.py
Line 35: Update FloatComp.parts() return type
Line 58: Update ExceptionRaisingComp.parts() return type
- test/formatters/test_template_formatter.py
Line 83: Update _AudioComponent.parts() return type
Line 194: Update _NoTemplate.parts() return type
|
I think I like ContentItem better 🤔 easy enough to change to make will wait until we have a additional feedback. cc @jakelorocco not sure if you have any strong opinions on naming. |
|
I agree that Having said that, I am also terrible at naming things (as a kid my pet birds were named "Blue Bird" and "Red Bird", and they were both green), so the best I can come up with is |
planetf1
left a comment
planetf1
left a comment
There was a problem hiding this comment.
Looks good — two small fixes needed before merge.
One thing I spotted that's pre-existing (not in this PR): backend.py lines 78 and 146 reference docs/dev/generate_signature_decisions.md, which doesn't exist — the actual file is docs/dev/generate_ctx_signature.md. Just flagging.
| """Mypy checks that SOFAI's S2 helpers accept the widened action union. | ||
|
|
||
| After #1439, `SOFAISamplingStrategy._extract_action_prompt`, | ||
| `_prepare_s2_context`, and `_generate_and_validate` accept `NodeData` |
There was a problem hiding this comment.
The docstring says _prepare_s2_context and _generate_and_validate are also checked here, but neither has a check function — only _extract_action_prompt does. Both of the others already accept NodeData, so adding checks is a copy-paste job. Or just remove the two names from the docstring if you'd rather keep it minimal.
There was a problem hiding this comment.
Added checks.
| from typing import Any | ||
|
|
||
| from ...core import CBlock, Component, ModelOutputThunk | ||
| from ...core import CBlock, Component, ModelOutputThunk, NodeData |
There was a problem hiding this comment.
Worth a quick comment here explaining that CBlock, Component, and ModelOutputThunk are retained for issubclass/match arms (lines 39–40 and 78–80), not just the union type hint — so this isn't an incomplete migration. Without it, someone will likely come along later and try to remove them.
There was a problem hiding this comment.
Added comment.
|
|
||
| model_input: CBlock | Component | ModelOutputThunk | None | ||
| model_input: NodeData | None | ||
| output: ModelOutputThunk | Component | None |
There was a problem hiding this comment.
output is ModelOutputThunk | Component | None rather than NodeData | None — CBlock is intentionally excluded since raw content blocks aren't valid model outputs. Worth a note in the docstring so the asymmetry with model_input above doesn't look accidental.
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
|
Review feedback is in, though I think this is still waiting on naming agreement / brainstorming. |
cc @jakelorocco |
| NodeData = Component | CBlock | ModelOutputThunk | ||
| """Canonical alias for the three node-shaped types that flow through mellea. | ||
|
|
||
| A `NodeData` is any of `Component`, `CBlock`, or `ModelOutputThunk` — the values | ||
| carried at each node of the context DAG, returned from `Component.parts()`, | ||
| rendered by formatters, passed to backends for generation, and sampled over by | ||
| sampling strategies. Only `Component` carries `.parse()`/`.format_for_llm()` | ||
| semantics; the other two are carried through as opaque content spans. | ||
| """ |
There was a problem hiding this comment.
@nrfulton can you please weigh on what you'd like to name this joint type. I was thinking something like "Action" (which corresponds to what can be generated from / goes into aact would make sense.
6 participants
Pull Request
Issue
Fixes #1439
Description
Introduces a new alias (NodeData) for the Component | Cblock | ModelOutputThunk union.
Also removes old imports no longer necessary based on this refactor.
New name open to discussion.
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.