Repository navigation
fix: improve genstubs and some related bugs - #1781
Conversation
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com> Assisted-by: CLAUDE:OPUS
bb6d27e to
b8f80d6
Compare
ajbozarth
left a comment
ajbozarth
left a comment
There was a problem hiding this comment.
Some feedback from Claude. LGTM — template redesign matches what #1476 scoped (and the bare-return-type rendering planetf1 anticipated), and the bundled None-result and template-lookup fixes are correct. Two optional, non-blocking notes inline.
| # import here would be circular. | ||
| from ..stdlib.components.genstub import GenerativeStub | ||
|
|
||
| assert isinstance(self._call.action, GenerativeStub), ( |
There was a problem hiding this comment.
Optional / genuine question, not a blocker: this bakes a specific stdlib subclass (GenerativeStub) into a core invariant purely to exempt it. Unlike the Message._parse dispatch just below, which does real parsing work, this one only exists to special-case one component type. Would a Component-level predicate (the component declaring it permits a None parsed_repr) read better here, or is naming GenerativeStub directly the deliberate call? Fine to leave as-is either way.
There was a problem hiding this comment.
Agreed; this was a bit code-smelly when I added it. I've opened #1805 to fix the actual issue which is that we can't distinguish not parsed from parsed nothing.
| mfuncs.act(_ParsesToNone("describe"), SimpleContext(), backend) | ||
|
|
||
|
|
||
| def test_sync_stub_returns_none_result(): |
There was a problem hiding this comment.
Optional: the relaxed invariant's safety rests on a non-nullable stub (-> str) still failing on {"result": null} — pydantic rejects it in _parse before the None check. Confirmed that holds, but there's no test pinning it: we test non-stub→None fails and nullable-stub→None succeeds, but not nullable-vs-non-nullable discrimination for stubs. A one-liner (@generative def f() -> str + a backend returning {"result": null}, expecting a raise) would lock in the exact property this change leans on.
Assisted-by: CLAUDE:OPUS Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
7b8dd45
Pull Request
Issue
Fixes #1476
Description
Improves genstub performance by modifying the template. On set of genstub tasks based on our examples and other scenarios, performance improved by 3.1% averaged across models and sizes. Granite models saw large improvements along with models with thinking on.
Performance saw large improvements when including the return format in the prompt.
Additional bug fixes:
-> X | Nonestubs crashed when the right answer wasNone. When the model correctly returned {"result": null}, a check in mellea/core/base.py required every result to be non-None and raised an AssertionError.a. docstrings left out when empty instead of rendering as
""""""b. argument types fixed
c. removed module paths from types
d. pydantic arguments are rendered as json along with dicts and lists
e. unquoted unresolved return types to prevent small models from returning strings
f. multiline docstrings keep their indentation
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.