refactor(core): promote ModelOutputThunk.thinking and deprecate _thin…#1315
refactor(core): promote ModelOutputThunk.thinking and deprecate _thin…#1315exequeryphil wants to merge 2 commits into
Conversation
…king alias Signed-off-by: Phil Williams <phil.williams@ibm.com>
|
Looks good. A minor issue is reported. Missing: Test for deprecated setter with warning The tests verify reading _thinking generates a deprecation warning, but there's no test verifying the setter also warns. The implementation includes both getter and setter (which is correct), but the test only covers the getter path: Current tests cover: Recommendation: Add one small test (no need for separate test, can extend existing): |
Signed-off-by: Phil Williams <phil.williams@ibm.com>
|
@exequeryphil Please run: Thanks! |
ajbozarth
left a comment
There was a problem hiding this comment.
This is looking solid, but I'm actually circling back on a design decision before we merge this. @planetf1 @jakelorocco I'm no longer sure it's worth including a deprecation shim for a private var like _thinking. I haven't be including deprecation shims for private vars in #1191 or my planned implementation for #1215 that I'm in the middle of.
If we do decide to keep the deprecation shim there are a few nits I'd raise, which I'll include a comments below
| assert mot.thinking == "reasoning trace" | ||
|
|
||
|
|
||
| def test_mot__thinking_deprecated_alias_warns_on_read(): |
There was a problem hiding this comment.
| def test_mot__thinking_deprecated_alias_warns_on_read(): | |
| def test_mot_thinking_deprecated_alias_warns_on_read(): |
no need to double underscore here, it just makes it less readable
| assert mot._thinking == "reasoning trace" | ||
|
|
||
|
|
||
| def test_mot__thinking_deprecated_alias_write_sets_public_field(): |
There was a problem hiding this comment.
| def test_mot__thinking_deprecated_alias_write_sets_public_field(): | |
| def test_mot_thinking_deprecated_alias_write_sets_public_field(): |
same here
|
|
||
| @property | ||
| def _thinking(self) -> str | None: | ||
| """Deprecated alias for :attr:`thinking`. |
There was a problem hiding this comment.
IIRC I don't think this is the correct docstring format for the project (Google style)
| return self.thinking | ||
|
|
||
| @_thinking.setter | ||
| def _thinking(self, value: str | None) -> None: |
There was a problem hiding this comment.
This should have the same warn as read
Pull Request
Issue
Fixes #1217
Description
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.