fix(emitter): keep an unset group_id unset when cloning - #1582
Open
LHMQ878 wants to merge 1 commit into
Open
Conversation
`Emitter.clone()` passed the group id positionally through `str()`, so the default `None` became the literal string `"None"`. `_create_event()` stamps `group_id=self._group_id` onto every `EventMeta` and `child()` falls back to it, so the fake value propagated to every event emitted by the clone and by every emitter descended from it: `is None` returned `False` and truthiness returned `True`. Agents are the common case here. All nine `clone()` implementations do `cloned.emitter = await self.emitter.clone()`, and agents build their emitter via `Emitter.root().child(namespace=[...], creator=self, events=...)` with no group id, so the broken path is the normal one. Pass the arguments by keyword and drop the coercion. `creator` no longer needs the `if self.creator else None` guard — it is already `object | None`. The existing `test_clone` constructs with `group_id="test_group"`, and a real group id round-trips through `str()` unchanged, which is why the default path was never covered. The TypeScript emitter does `this.groupId = input?.groupId` and preserves the absent value, so this was a Python-only divergence. Signed-off-by: LHMQ878 <72402929@cityu-dg.edu.cn>
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue(s) does this pull-request address?
Closes: #1581
Description
Emitter.clone()built the clone with the group id passed positionally throughstr():group_idisstr | Noneand defaults toNone, so when it was unset the clone received the literal string"None"._create_event()stampsgroup_id=self._group_idonto everyEventMeta, andchild()doesgroup_id=group_id or self._group_id, so the synthetic value propagated to every event emitted by the clone and by every emitter descended from it.Measured on
main(21284d7, Python 3.12.10):Emitter(namespace=["app"])._group_idNoneNone(await that.clone())._group_id'None'Nonecl._group_id is NoneFalseTruebool(cl._group_id)TrueFalseevent.group_idfrom the clone'None'None(await that.clone()).child(...)._group_id'None'Nonegroup_id="run-42"'run-42''run-42'Agents are the common case rather than an edge case. All nine
clone()implementations docloned.emitter = await self.emitter.clone()— the four adapter agents (a2a,acp,agentstack,watsonx_orchestrate), the four core agents (lite,react,requirement,tool_calling), andcontext.py:281— and agents build their emitter viaEmitter.root().child(namespace=["agent", "tool_calling"], creator=self, events=...)with no group id. Events off a cloned agent looked like this:Anything grouping or filtering by
group_idtherefore saw a spurious"None"group for cloned agents:if meta.group_id:became true,meta.group_id is Nonebecame false, and a dict keyed by group id gained a"None"bucket. Forwardingmeta.group_idto a tracing backend recorded the string as-is.The fix passes the constructor arguments by keyword and drops the coercion, so the absent value stays absent.
creatorno longer needs theif self.creator else Noneguard — the attribute is alreadyobject | None, so the conditional was a no-op.The TypeScript emitter does
this.groupId = input?.groupId(typescript/src/emitter/emitter.ts:60) and preserves the absent value throughchild()(:80) andcreateSnapshot()(:254), so this was a Python-only divergence and no TypeScript change is needed.Why the existing test missed it
test_cloneconstructs withgroup_id="test_group", and a real group id round-trips throughstr()unchanged, so the only broken path — the defaultNone— was never exercised. Four tests are added next to it:test_clone_keeps_absent_group_id_absentAssertionError: assert 'None' is Nonetest_clone_emits_events_without_a_group_idassert ['None'] == [None]test_child_of_clone_inherits_absent_group_idAssertionError: assert 'None' is Nonetest_clone_preserves_group_idChecklist
General
/ [ ] TypeScript
Pythonfor Python changes,TypeScriptfor TypeScript changes — I cannot set labels on this repo; please addPython.Code quality checks
ruff checkandruff format --checkare clean on both changed files.mypy beeai_framework/emitter/emitter.pyreports only four pre-existing findings unrelated to this diff (missingdeprecated/cachetoolsstubs,utils/schema.py:67,backend/chat.py:489) — identical with the change reverted.Testing
Unit tests pass —
python -m pytest -m unit, run on Windows/Python 3.12.10:The 3 resolved failures are the new tests. The 1 remaining failure is the same in both runs and unrelated:
tests/tools/filesystem/test_read_edit.py::test_tools_route_through_installed_backendfails withToolInputValidationError: 'path' must be absolute, got: '/tmp/virtual.txt'— a POSIX path that is not absolute on Windows, so it is a platform artefact of my environment, not a regression.Two collection paths are excluded in my environment and in both runs equally:
beeai_framework/adapters/transformersandtests/backend/providers/test_transformers.py, which fail to import withModuleNotFoundError: No module named 'outlines_core.fsm'from the installedoutlinespackage.E2E tests pass:
mise test:e2e— not run; they require model-provider credentials I do not have. This change touches no provider code path.Tests are included (for bug fixes or new features)
Documentation
str | Nonecontract rather than altering it.