Conversation
When group_id is None, str(None) produces the literal string 'None', corrupting every event emitted by the cloned emitter and its descendants. Pass group_id by keyword argument instead, so None stays None. Fixes i-am-bee#1581
Updates requires-python from >=3.11,<3.14 to >=3.11,<3.15 to support Python 3.14 installations. Fixes i-am-bee#1588
|
Thanks @re2zero! A few points before this can move forward:
Appreciate you pushing on 3.14 — it's wanted. 🐝 |
|
Update: the emitter |
Tomas2D
left a comment
There was a problem hiding this comment.
Thanks — and note this PR is doing more than the title suggests, in a good way.
The Emitter.clone() change is a real bug fix independent of the version bump: cloning passed arguments positionally and wrapped the group id in str(...), so an emitter with _group_id is None came back from clone() with the literal string "None" as its group id, which then leaked into every EventMeta.group_id. The switch to keyword arguments plus the regression test is correct. Consider calling that out in the PR title/description (or splitting it), since it's worth backporting attention on its own.
Two things before this can go in:
- The branch has conflicts with
main(mergeable: CONFLICTING) — please rebase. - Justify the
<3.15bound. Per #1588,litellmnow declares<3.15, which was the original blocker, but relaxing our own bound only helps if the whole dependency tree resolves and the suite is green on 3.14. Ideally CI should actually run a 3.14 job as part of this change — otherwise we'd be advertising support we don't test. If adding a 3.14 matrix entry is out of scope here, please at least paste a full unit-suite run on 3.14.
Related: #1588.
|
Correction to my earlier review — I was wrong about the I said it was a real bug fix worth calling out separately in the title or splitting into its own PR. It isn't: that fix already landed on I misread the diff — its context lines reflect this branch's merge base rather than current So the actual ask is the opposite of what I wrote. On rebase onto current
My second point stands unchanged: relaxing the bound only helps if the whole dependency tree resolves and the suite is green on 3.14, so this should come with a 3.14 CI matrix entry or a pasted full unit-suite run on 3.14. Sorry for the detour. |
Tomas2D
left a comment
There was a problem hiding this comment.
Thanks for taking this on. Two separate things here, and I think the PR is blocked on something upstream rather than on anything you can fix in this diff.
Relaxing requires-python alone isn't enough — litellm caps at <3.14.
Grepping the current lock file for deps that exclude 3.14:
| package | python-versions |
optional? |
|---|---|---|
| litellm | <3.14,>=3.10 |
no — core dependency |
| agentstack-sdk | <3.14,>=3.11 |
yes (agentstack, beeai-platform) |
| unstructured | <3.14,>=3.11 |
yes |
| outlines | <3.14,>=3.10 |
yes |
| cz-commitizen | >=3.11,<3.14 |
dev |
litellm is declared as litellm = "^1.84.0" in [tool.poetry.dependencies] with no optional = true, so it's pulled in by every install. Widening requires-python to <3.15 makes the framework's metadata claim 3.14 support, but poetry lock then has to resolve a core dep that refuses to install on it. Until litellm ships a release that allows 3.14, this can't actually work — the constraint here is the symptom, not the cause.
Immediate CI failure (log):
pyproject.toml changed significantly since poetry.lock was last generated. Run `poetry lock` to fix the lock file.
poetry.lock still carries python-versions = ">= 3.11,<3.14". It isn't in the diff, so it never got regenerated — which is also what would have surfaced the litellm conflict above locally.
The emitter change is a real fix and deserves its own PR. Emitter.clone() passing str(self._group_id) positionally turns a None group id into the literal string "None", and your test_clone_without_group_id pins exactly that. Switching to keyword arguments fixes it and makes the call robust to signature changes. That's unrelated to the version constraint, it's independently mergeable, and it's currently stuck behind a blocked upstream dependency. Splitting it out would get it landed now.
The branch also has conflicts with main at this point.
|
Status update, since this has been sitting a while and one half of it is now moot. The cloned = Emitter(
group_id=self._group_id,
namespace=self.namespace.copy(),
...So the What remains is the Given that, I'd suggest closing this in favour of tracking the upstream litellm release on #1588, unless you'd prefer to keep it open as a placeholder. If litellm does ship 3.14 support, the constraint bump plus a regenerated Thanks for the emitter catch either way; it was a real bug. |
Updates
requires-pythonfrom>=3.11,<3.14to>=3.11,<3.15to support Python 3.14 installations.Fixes #1588