chore(sdk): migrate legacy rhesis.sdk.telemetry imports to rhesis.telemetry - #2490
Conversation
PR #2462 moved attributes, context and token_extraction into the light rhesis package so framework integrations can depend on rhesis[telemetry] rather than the whole SDK, and left re-export shims at the old rhesis.sdk.telemetry paths. Every import site in this repository still went through those shims, which made the canonical path the exception rather than the rule and kept the old ones looking load-bearing. Point them at the canonical modules: rhesis.telemetry.attributes, rhesis.telemetry.context and rhesis.telemetry.token_extraction. Note the path flattens — utils.token_extraction becomes token_extraction. Two patch targets in tests/backend/tasks/architect/test_chat.py move with the lazy import they patch, since a mock on the shim would no longer be seen by code importing from the canonical module. The shims stay, for consumers outside this repository — released rhesis-haystack versions import them — and their docstrings now say that rather than counting internal call sites. rhesis.sdk.telemetry.utils keeps re-exporting extract_token_usage through the shim on purpose: that package is itself the old public surface. Closes #2473 Signed-off-by: Arman Beykmohammadi <arman.beykmohammadi@rhesis.ai>
Nothing in this repository imports rhesis.sdk.telemetry.attributes, .context or .utils.token_extraction any more, so the shims that keep them working are now untested — a compatibility surface no test touches is one that breaks silently. Released rhesis-haystack versions import two of them. Assert what the shims actually promise: every name they advertise is the canonical object rather than a copy, a public symbol added to a canonical module has to be forwarded, and the ContextVars are shared across both paths, since that identity is what lets the SDK and a framework integration read the same turn state while importing from different places. Signed-off-by: Arman Beykmohammadi <arman.beykmohammadi@rhesis.ai>
rhesis-sdk ships rhesis/py.typed at the namespace root, which is what makes type checkers accept rhesis.telemetry.* — including the modules that now live in the light package. Anything depending on rhesis[telemetry] without the SDK gets no marker, so a type checker skips those imports entirely: the Haystack integration's mypy run reports 11 import-untyped errors the moment it drops rhesis-sdk, on modules that are fully annotated. The package ships annotations, so declare it typed. The file sits beside rhesis/__init__.py, matching where rhesis-sdk puts its own — both distributions already contribute files at that path, so this adds no new kind of overlap. Confirmed present in the built wheel. Signed-off-by: Arman Beykmohammadi <arman.beykmohammadi@rhesis.ai>
![peqy[bot]](https://avatars.githubusercontent.com/in/1930020?s=60&v=4){kind=small}
There was a problem hiding this comment.
[Improvement] The new legacy-shim tests are a great safety net. One tweak: test_shim_forwards_everything_public currently ignores public primitive constants in the canonical modules (since they don’t have __module__), so a future constant could be added without the test forcing it into the shim.
Everything else in this PR looks consistent with the migration goal, and adding py.typed to the light package should unblock downstream type checking.
Found 1 issue (0 critical, 1 improvement).
c9df430 to
32bfc6a
Compare
There was a problem hiding this comment.
Overall this looks good: the repo now consistently imports telemetry from rhesis.telemetry.*, the shim modules are clearly documented as legacy-only, and adding packages/rhesis/src/rhesis/py.typed should fix downstream type checking.
Only remaining item is the legacy-shim pinning test: test_shim_forwards_everything_public currently misses in-module primitive constants (e.g. MAX_CONTENT_LENGTH in rhesis.telemetry.attributes), so the shim surface could still drift silently. See thread for suggested fixes.
The drift check compared the shim against public names inferred from the canonical module's namespace, filtered by __module__. That filter catches functions and classes and silently drops constants: MAX_CONTENT_LENGTH is an int and carries no __module__, so the check was blind to exactly the kind of symbol most likely to be added later — and dropping the filter instead would have swept in imports the module makes for its own use, like FORBIDDEN_SPAN_DOMAINS from schemas. Inference cannot tell those apart. Declare the surface instead: __all__ on rhesis.telemetry.attributes, .context and .token_extraction, which the shims must now mirror exactly. Both directions are asserted, so a name added to a canonical module fails the test until someone decides whether it belongs on the old path too. Verified the new check bites where the old one did not: appending a constant to a canonical __all__ fails it, while the old heuristic did not even see MAX_CONTENT_LENGTH. Signed-off-by: Arman Beykmohammadi <arman.beykmohammadi@rhesis.ai>
|
Looks good overall; the new canonical [Improvement]
[Improvement] One internal legacy import still appears to remain
Found 2 issues (0 critical, 2 improvements). |
Missed by the sweep in the first commit: that pass rewrote the working tree, and this file already carried the canonical import there as uncommitted work, so the grep came back clean while the committed version still went through the shim. Signed-off-by: Arman Beykmohammadi <arman.beykmohammadi@rhesis.ai>
|
Thanks — one real, one already covered. The travel-agent import: you are right, fixed in a6bbb9e. Worth recording why it slipped, because the reason is the interesting part: my sweep rewrote the working tree, and that file has uncommitted work of mine in which the import was already canonical. So the grep came back clean while the committed version still went through the shim. Re-ran the check against committed content this time ( py.typed: confirmed shipping, no build change needed. I checked the artifacts rather than the config, since that is the only thing that settles it.
Independently: mypy on the Haystack integration passes against a wheel install of this package in a clean venv (not an editable install), which it cannot do unless the marker is really in the wheel. hatchling includes non-Python files under |
There was a problem hiding this comment.
Looks good; the canonical __all__ + shim-surface test is a solid way to pin the compatibility layer, and the last internal legacy import in travel-agent is now migrated.
One remaining (non-blocking) suggestion from my earlier note: consider explicitly including src/rhesis/py.typed in packages/rhesis wheel build config as a belt-and-suspenders measure (mirrors the SDK’s hatch config), unless you’re 100% confident hatchling is already packaging it by default.
hatchling already ships the marker: with packages = ["src/rhesis"] it takes everything under that directory, and both artifacts carry it — the wheel as rhesis/py.typed, the sdist as src/rhesis/py.typed. Building wheel and sdist with and without this line produces identical file sets, 14 and 16 entries either way, so this changes no output today. What it buys is that the marker stops depending on that default. An exclude pattern or a narrowed packages list would silently drop it again, and the only symptom would be a downstream type checker quietly skipping rhesis.telemetry.* — which is exactly the failure this marker was added to fix. The SDK's wheel config lists its py.typed the same way, so this also makes the two packages read alike. Signed-off-by: Arman Beykmohammadi <arman.beykmohammadi@rhesis.ai>
|
Follow-up on latest commits:
No further issues from my side. |
|
Took the I wanted to settle the "unless you are 100% confident" part with a measurement rather than an argument, so I built wheel and sdist both ways and compared file sets: identical, 14 entries in the wheel and 15 in the sdist either way, marker present in both. So my earlier worry that an That makes the line free, and it buys something real: the marker stops depending on the Both of your notes from that review are now closed. Full suites re-run on the final state: 2429 sdk tests, 574 backend tests, all green. |
1 participant
Closes #2473
What
Point every telemetry import site in this repository at the canonical
rhesis.telemetry.*modules, keep therhesis.sdk.telemetry.*shims for outside consumers, and givethe light package the
py.typedmarker it turns out to need.Why
#2462 moved
attributes,contextandtoken_extractioninto the lightrhesispackage soframework integrations can depend on
rhesis[telemetry]instead of the whole SDK, and leftre-export shims behind. Every call site here still went through the shims, which made the canonical
path the exception and kept the old ones looking load-bearing.
Changes
chore(sdk): import telemetry primitives from rhesis.telemetry— 43 files acrosssdk/,apps/backend/,tests/,agents/,examples/anddocs/. The path flattens on the way:utils.token_extraction→token_extraction. Two mock targets intests/backend/tasks/architect/test_chat.pymove with the lazy import they patch — a mock on theshim would no longer be seen by code importing from the canonical module.
The grep from the issue now returns exactly one line, in
sdk/src/rhesis/sdk/telemetry/utils/__init__.py,which keeps re-exporting
extract_token_usagethrough the shim deliberately: that package is itselfthe old public surface, and there is now a comment saying so. The shim docstrings say they exist for
consumers outside this repository rather than counting internal call sites.
test(sdk): pin the legacy telemetry import shims— nothing here exercises the old paths anymore, and a compatibility surface no test touches is one that breaks silently. Released
rhesis-haystackversions import two of them. The test asserts that each advertised name is thecanonical object rather than a copy, that a public symbol added to a canonical module must be
forwarded, and that the ContextVars are shared across both paths — that identity is what lets the SDK
and a framework integration read the same turn state while importing from different places.
fix(telemetry): ship a py.typed marker in the light package— not in the issue, found whiledoing the matching swap in the Haystack integration.
rhesis-sdkshipsrhesis/py.typedat thenamespace root, and that marker is what makes type checkers accept
rhesis.telemetry.*. Depend onrhesis[telemetry]without the SDK and there is no marker, so a type checker skips those imports:the integration's mypy run reports 11
import-untypederrors on fully annotated modules the momentit drops
rhesis-sdk. Confirmed present in the built wheel.Testing
make -C sdk test— 2429 passed, 16 skippedtests/backend/{services/architect,tasks,services/telemetry}— 574 passed, 3 skippedagents/visit-prepandagents/travel-agentspan-tree tests — 5 and 3 passedNote for the reviewer
The
py.typedcommit is what unblocksdeepset-ai/haystack-core-integrations#3669,
which now depends on
rhesis[telemetry]>=0.13.0instead ofrhesis-sdk. That integration needs arelease of the light package carrying both this marker and the modules #2462 moved — published
rhesis0.12.0 predates the move.