Add async session boundary to the Python SDK (FABRIC-10) - #12
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multi-turn session support to the NeMo-Fabric Python SDK via Session, SessionStatus, and FabricSessionUnsupportedError types in client.py; FabricClient.start() and start_config() as native-extension-only entry points; Hermes adapter request-scoped history resolution; full pytest and smoke test coverage; quickstart example; Hermes session profile; README documentation; and CI registration. ChangesMulti-turn Session SDK
Sequence Diagram(s)sequenceDiagram
participant Caller
participant FabricClient
participant _make_session
participant Session
participant _run_inline_adapter
Caller->>FabricClient: start(path, profile, overrides)
FabricClient->>FabricClient: resolve plan via native extension
FabricClient->>_make_session: plan, client, overrides
_make_session->>_make_session: validate inline Python entrypoint
_make_session-->>Caller: Session(status=ACTIVE)
Caller->>Session: invoke(input, overrides)
Session->>Session: merge all override levels
Session->>Session: build context with accumulated history
Session->>_run_inline_adapter: plan, request, entrypoint
_run_inline_adapter-->>Session: result (output, session_id, handles)
Session->>Session: absorb messages/invocations, update id
Session-->>Caller: result
Caller->>Session: stream(input, overrides)
Session->>Session: invoke internally
Session->>Caller: yield events sequentially
Session->>Caller: yield terminal succeeded result
Caller->>Session: cancel()
Session->>Session: cancel in-flight task (CancelledError)
Session->>Session: status = CANCELLED
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
e45fb60 to
1bcdf23
Compare
…cel/stop) Finish the async SDK boundary beyond the stable oneshot `run`: a client-side multi-turn Session over the existing stateless adapter call. No new wire schemas -- turns reuse RunRequest / RunResult. - FabricClient.start / start_config open a Session (FabricSessionUnsupportedError when the resolved adapter is not session-capable). - Session.invoke runs one turn, replaying the accumulated transcript as request.context.history and merging per-turn overrides. - Session.stream yields normalized events then the final RunResult (buffered; the async-iterator shape is forward-compatible with token streaming). - Session.cancel cooperatively aborts the in-flight turn (idle or running) and marks the session cancelled; Session.stop finalizes (idempotent, async cm). - hermes-sdk adapter: resolve_history threads request.context.history into run_conversation (request context wins over static settings; oneshot unchanged). Session state is client-side (transcript replayed as history); a persistent, harness-stateful session is a later phase, bounded by Hermes support. Tests: python/tests/smoke_sdk_sessions.py (dependency-free; fakes the inline adapter to assert multi-turn threading, buffered stream, cooperative cancel, idempotent stop, gating). tests/smoke_hermes_session.py (opt-in real-Hermes multi-turn, gated by RUN_FABRIC_HERMES_INTEGRATION). examples/session_quickstart.py. Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Add the dependency-free session smoke (smoke_sdk_sessions) to the explicit CI smoke list so the Session boundary is enforced on every PR. The real-Hermes session smoke stays gated (RUN_FABRIC_HERMES_INTEGRATION) and out of CI. Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Track {request_id, runtime_id, invocation_id} per turn and expose them via
Session.invocations, so a session is correlatable to its runtimes, telemetry,
and artifacts even when it spans multiple runtimes (one per turn) -- the case
when the harness has no resumable runtime (e.g. Hermes). Aligns the session
surface toward the runtime-mode contract without faking a persistent runtime.
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
74aa461 to
37755fb
Compare
Add Args/Returns/Raises (and Yields for stream) to the session surface so the generated API reference documents the full contract: start, start_config, Session.invoke, Session.stream, and the previously-undocumented Session.info. Records the raised errors per method (FabricNativeUnavailableError, FabricSessionUnsupportedError, RuntimeError on a non-active session). The full-SDK docstring pass + Fern regeneration is FABRIC-2 / PR #7. Signed-off-by: Ajay Thorve <athorve@nvidia.com>
pytest unit tests for the Session boundary (start/invoke/stream/cancel/stop), matching the suite added in #13: dependency-free via a monkeypatched inline adapter. Covers history replay, per-turn override merge, invocation-handle correlation, output.session_id adoption, buffered stream, idempotent stop + context manager, cooperative cancel (idle and in-flight), the info summary, defensive-copy semantics of messages/invocations, an empty-output turn, adapter gating, and the native-required error. Auto-discovered by the CI `uv run pytest`. Signed-off-by: Ajay Thorve <athorve@nvidia.com>
2f591fe to
3a4ae87
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
README.md (1)
190-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent session.status formatting with quickstart example.
The README code accesses
session.statusdirectly (which prints the enum object), butexamples/session_quickstart.py:32usessession.status.valueto show the enum string value. For consistency and cleaner output, use.value:print(session.id, session.status.value, len(session.messages))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 190, The print statement accessing session.status should use session.status.value instead to access the enum string value rather than the enum object representation. This ensures consistency with the examples/session_quickstart.py file and produces cleaner output. Update the print statement that displays session.id, session.status, and len(session.messages) by changing session.status to session.status.value.tests/test_session.py (1)
78-197: 📐 Maintainability & Code Quality | 🔵 TrivialAsync tests are properly configured via
asyncio_mode = "auto".The repository intentionally enables async test discovery in
pyproject.toml:40. All 15 unmarkedasync def test_*functions in this file will run correctly without explicit decorators. Adding@pytest.mark.asynciodecorators is optional but would improve clarity and self-documentation of test intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_session.py` around lines 78 - 197, Add the `@pytest.mark.asyncio` decorator to all async test functions in the file to improve clarity and self-document test intent. Place the decorator immediately above each async def test_* function signature for the following functions: test_new_session_is_active_and_empty, test_invoke_threads_accumulated_history, test_invoke_records_per_turn_handles, test_invoke_adopts_session_id_from_output, test_invoke_merges_session_and_turn_overrides, test_stream_yields_events_then_result, test_stop_is_idempotent_and_blocks_invoke, test_context_manager_auto_stops, test_cancel_when_idle_marks_cancelled, test_cancel_aborts_in_flight_turn, test_make_session_rejects_non_session_adapter, and test_start_requires_native_extension. While these tests will run correctly with the current asyncio_mode configuration, explicit decorators improve readability and make async test intent clearer to other developers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py`:
- Around line 78-79: The issue is that using the `or` operator in the return
statement treats empty lists as falsy, so when a request explicitly sets history
to an empty list to clear it, the code incorrectly falls back to the static
harness history. Instead of using `context.get("history") or ...`, check for key
presence explicitly by verifying if "history" exists in the context dictionary
before falling back to settings_payload. Replace the `or` logic with a
conditional that returns context["history"] if the key exists (regardless of its
value being empty or not), and only falls back to
settings_payload(payload).get("history") when the key is actually absent from
context.
In `@python/src/nemo_fabric/client.py`:
- Around line 412-413: The session class has a race condition where concurrent
invoke() or stream() calls can simultaneously replay the same transcript,
overwrite _current_task, and compete in _absorb(), causing turns to be lost or
reordered. Add an asyncio.Lock as an instance attribute in the session
initialization (alongside _current_task and id) to serialize access to critical
sections. Use this lock to guard all turn replay and mutation operations in
invoke(), stream(), and _absorb() methods to ensure only one call processes
turns at a time.
- Line 24: The issue is that stale context.history from incoming requests can
bypass the authoritative accumulated session transcript (around line 482),
existing request overrides can bypass the documented session/per-turn merge
logic (around line 486), and message objects are stored and exposed by reference
which allows callers or adapters to mutate them and corrupt future turns. To fix
this, ensure the session's accumulated transcript is always authoritative by not
allowing incoming request context.history to override it, enforce the documented
merge logic for overrides rather than letting request overrides bypass it, and
create deep copies of all message objects before storing them (in methods around
lines 420-423, 482-486, 570-572) so mutations to caller-held references do not
affect the session state.
In `@README.md`:
- Around line 181-194: The code example in the README is missing the `import
asyncio` statement at the top. The example defines an async function `chat()`
and then calls it with `asyncio.run(chat())` at the end, but the asyncio module
is never imported. Add `import asyncio` as the first import statement before the
`from nemo_fabric import FabricClient` line to resolve the missing dependency.
---
Nitpick comments:
In `@README.md`:
- Line 190: The print statement accessing session.status should use
session.status.value instead to access the enum string value rather than the
enum object representation. This ensures consistency with the
examples/session_quickstart.py file and produces cleaner output. Update the
print statement that displays session.id, session.status, and
len(session.messages) by changing session.status to session.status.value.
In `@tests/test_session.py`:
- Around line 78-197: Add the `@pytest.mark.asyncio` decorator to all async test
functions in the file to improve clarity and self-document test intent. Place
the decorator immediately above each async def test_* function signature for the
following functions: test_new_session_is_active_and_empty,
test_invoke_threads_accumulated_history, test_invoke_records_per_turn_handles,
test_invoke_adopts_session_id_from_output,
test_invoke_merges_session_and_turn_overrides,
test_stream_yields_events_then_result,
test_stop_is_idempotent_and_blocks_invoke, test_context_manager_auto_stops,
test_cancel_when_idle_marks_cancelled, test_cancel_aborts_in_flight_turn,
test_make_session_rejects_non_session_adapter, and
test_start_requires_native_extension. While these tests will run correctly with
the current asyncio_mode configuration, explicit decorators improve readability
and make async test intent clearer to other developers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f731c5b1-f57f-4338-91d7-759948d8475c
📒 Files selected for processing (9)
.github/workflows/ci_python.ymlREADME.mdadapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.pyexamples/session_quickstart.pypython/src/nemo_fabric/__init__.pypython/src/nemo_fabric/client.pypython/tests/smoke_sdk_sessions.pytests/smoke_hermes_session.pytests/test_session.py
The example's hermes_sdk/cli/relay profiles override the base agent to runtime.mode: oneshot, so the session quickstart/smoke previously ran over a oneshot-declared config. Add a hermes_session profile (hermes-sdk adapter, runtime.mode: session) and point the quickstart, the gated real-Hermes smoke, and the README session snippet at it, so the example demonstrates session mode honestly. Document that sessions are SDK-only (no fabric CLI equivalent). Signed-off-by: Ajay Thorve <athorve@nvidia.com>
- adapter resolve_history: check `history` key presence so an explicit empty history ([]) clears the conversation instead of falling back to static settings. - Session: the accumulated transcript is authoritative (thread it over any caller-supplied request.context.history); request-level overrides are merged (session < request < per-turn) rather than bypassing the merge; messages are deep-copied on store, expose, and thread so caller/adapter mutation cannot corrupt the session. - README: use session.status.value for consistency with the quickstart. - tests: cover authoritative history, request-override merge, and deep-copy isolation. (Skipped the @pytest.mark.asyncio nitpick: asyncio_mode=auto is the repo convention and the #13 suite omits the decorator.) Signed-off-by: Ajay Thorve <athorve@nvidia.com>
agent_profile was set in agent.yaml and every hermes profile but is read by neither the adapters nor the core, so it was inert. Drop it. Signed-off-by: Ajay Thorve <athorve@nvidia.com>
- Session.invoke claims an in-flight guard before any await, so concurrent invoke()/stream() calls on one session are rejected (turns are ordered) rather than racing the transcript and _absorb(). - README session example: add the missing `import asyncio`. - test: a second concurrent invoke is rejected while a turn is in flight. Signed-off-by: Ajay Thorve <athorve@nvidia.com>
Signed-off-by: Ajay Thorve <athorve@nvidia.com>
33ebaa3 to
0b02612
Compare
Implements the async SDK session boundary (FABRIC-10) — the remaining runtime-mode work beyond the stable oneshot
run.What
A client-side multi-turn
Sessionlayered over the existing stateless adapter call. No new wire schemas — turns reuseRunRequest/RunResult.start/start_configSession; raiseFabricSessionUnsupportedErrorif the adapter is not session-capableinvokerequest.context.history; merges per-turnoverridesstreamevents, then the finalRunResult(buffered; async-iterator shape is token-stream-ready)cancelstopSessionexposesid,status,messages(transcript), andinfo. Thehermes-sdkadapter gainsresolve_historyso a turn'srequest.context.historyreachesrun_conversation(request context wins over static settings; oneshot behavior unchanged).Scope / bound
Session state is client-side — the transcript is replayed as conversation history each turn. A persistent, harness-stateful session is a later phase, bounded by Hermes support (FABRIC-20). This cut ships the function boundary with minimal semantics: buffered stream (no live token streaming), and cooperative cancel (the inline adapter runs in a worker thread that cannot be hard-killed; the process path can terminate). Session-support gating is by adapter kind today; an explicit adapter-descriptor capability flag is a follow-up.
Tests
python/tests/smoke_sdk_sessions.py— dependency-free (no native ext, no Hermes); fakes the inline adapter to assert multi-turn history threading, buffered stream, cooperative cancel (idle + in-flight), idempotent stop, and gating. Passing.tests/smoke_hermes_session.py— opt-in real-Hermes multi-turn memory check, gated byRUN_FABRIC_HERMES_INTEGRATION(needs Hermes +NVIDIA_API_KEY+ the native ext).examples/session_quickstart.py— start → invoke → stream → stop.Draft pending the related in-flight PRs (SDK guardrails, SDK API doc); will rebase as they land.
Summary by CodeRabbit
SessionStatus,Session, andFabricClient.start()/start_config()lifecycle controls.