Skip to content

feat(signal): add SignalManager to Python SDK#598

Open
RapidPoseidon wants to merge 1 commit into
mainfrom
feat(signal)/sdk-support
Open

feat(signal): add SignalManager to Python SDK#598
RapidPoseidon wants to merge 1 commit into
mainfrom
feat(signal)/sdk-support

Conversation

@RapidPoseidon
Copy link
Copy Markdown
Contributor

@RapidPoseidon RapidPoseidon commented May 21, 2026

Summary

Adds SDK support for the new Signal Service:

  • RapidataSignalManager exposed as client.signals (CRUD entry point: create, get, list, get_run).
  • RapidataSignal with lifecycle methods (pause, resume, delete, trigger, update), get_runs / get_run, and a blocking wait_for_next_run(timeout, poll_interval) helper that returns the first new terminal run.
  • SignalRun dataclass with is_terminal / succeeded convenience properties.

Notes for review

  • Backend service shipping in parallel — uses raw requests against the gateway until OpenAPI regen. HTTP currently goes through self._openapi_service.api_client.call_api(...) against https://api.{environment}/signal/... (same pattern RapidataClient._check_beta_features uses); the OAuth client transparently attaches the bearer token. To upgrade after backend deploy: add signal to the openapi generation list, then drop _signal_http.py and switch the manager / entity calls over to the typed client.
  • 4xx/5xx are converted to RapidataError with status code, message and (when available) parsed details — matching the existing error contract for the rest of the SDK.
  • Datetime parsing uses datetime.fromisoformat (Python 3.10 compatible — Z suffix is normalised to +00:00).
  • All re-exports added at both rapidata.rapidata_client and top-level rapidata namespaces.

Test plan

  • python -c "from rapidata import RapidataClient; print('OK')"
  • python -c "from rapidata.rapidata_client.signal import RapidataSignalManager, RapidataSignal, SignalRun; print('OK')"
  • python -c "from rapidata import RapidataSignal, RapidataSignalManager, SignalRun; print('OK')"
  • pyright src/rapidata/rapidata_client -> 0 errors

Backend PR:

🔗 Session: https://session-b2a2bd76.poseidon.rapidata.internal/

Adds SDK support for the new Signal Service:
- RapidataSignalManager exposed as `client.signals` for CRUD on signals.
- RapidataSignal with lifecycle methods (pause/resume/delete/trigger/update),
  run listing, and a blocking wait_for_next_run helper.
- SignalRun dataclass with terminal-status / success convenience properties.

Backend service is shipping in parallel, so HTTP goes through the existing
OAuth-authenticated api_client.call_api against /signal on the gateway. To
upgrade after the OpenAPI regen: drop _signal_http and switch to the typed
client.

Co-Authored-By: luca <luca@rapidata.ai>
@claude
Copy link
Copy Markdown

claude Bot commented May 21, 2026

Code Review — feat(signal): add SignalManager to Python SDK

Overview

This PR adds a RapidataSignalManager to the SDK, exposing a new Signal Service via a temporary raw-HTTP layer (_signal_http.py) until the OpenAPI client is regenerated. The design is clean and follows the existing manager/entity pattern well. A few issues below worth addressing before merge.


Bugs / Correctness

wait_for_next_run — misleading comment and potentially wrong return value
In rapidata_signal.py the inline comment says "the earliest new terminal — return that one (the next to finish)", but terminal[-1] on a descending-sorted list is actually the oldest new terminal run, not the one that finished most recently. If the caller just called trigger() and wants the result of that specific run, this can silently return a stale run. Consider returning terminal[0] (the newest) or clarifying the intent.

# Current
return terminal[-1]   # oldest new terminal (confusing intent)

# Likely intended
return terminal[0]    # newest terminal run (most recently started that finished)

wait_for_next_run — client/server clock skew
started_waiting = datetime.now(timezone.utc) is a local client timestamp. r.started_at comes from the server. If there's any clock skew (even a few seconds), runs that fired right around wait_for_next_run() being called can be filtered out permanently. A safer approach is to snapshot the run IDs present at call time and look for newly-appearing ones instead:

seen_ids = {r.id for r in self.get_runs(page=1, page_size=20)}
# then look for runs whose id is not in seen_ids

wait_for_next_run — hardcoded page_size=20
If more than 20 runs arrive between polls (e.g. after a burst of trigger() calls), new terminal runs will be silently missed. At minimum, expose page_size as a parameter or use the same value as the default in get_runs.


Style / Convention Violations

Double-underscore __openapi_service in RapidataSignalManager
Every other manager in the codebase uses self._openapi_service (single underscore). RapidataSignalManager uses self.__openapi_service (name-mangled). This is inconsistent and will cause AttributeError if anything accesses it via the base name. Change to _openapi_service to match the rest.

# All other managers
self._openapi_service = openapi_service  # ✓

# Signal manager — should match
self.__openapi_service = openapi_service  # ✗

Import inside function body (_signal_http.py, line ~55)
from urllib.parse import urlencode is imported inside the if params: block. Per PEP 8 and project conventions, imports belong at the top of the file.

Missing __repr__ on SignalRun
All other entity classes define both __str__ and __repr__ (with __repr__ delegating to __str__). SignalRun only defines __str__, so repr() falls back to the verbose dataclass default.


Missing Items (per CLAUDE.md)

No documentation update
CLAUDE.md states: "when updating any interfaces make sure you update the docs and examples." No docs changes are included in this diff. Even a brief entry in the relevant docs page for client.signals would satisfy this.

No automated tests
The test plan is entirely manual. While the temporary HTTP layer makes pure-unit tests harder to write, at minimum SignalRun._from_api, _parse_datetime (especially the Z-suffix normalization), and wait_for_next_run timeout logic can be unit-tested without network access.

No type annotation for self.signals in rapidata_client.py
All other public attributes on RapidataClient are typed. self.signals has no annotation:

# Add the annotation alongside the assignment
self.signals: RapidataSignalManager = RapidataSignalManager(
    openapi_service=self._openapi_service
)

Minor

  • update() cannot clear description (set it back to None) — the if description is not None guard prevents it. If unsetting is a valid use-case, consider a sentinel default (e.g. _UNSET = object()).
  • trigger() is the only mutating lifecycle method without a managed_print call. pause(), resume(), and delete() all print confirmation. Consider adding one for consistency.
  • The _parse_datetime / _parse_optional_datetime helpers are private (underscore-prefixed) but are re-exported across module boundaries inside the same package. If they are needed in rapidata_signal.py, moving them to a shared _utils.py inside the signal package would keep the private API cleaner.

Summary

Severity Count
Bug / Correctness 3
Convention violation 3
Missing required items (CLAUDE.md) 3
Minor 3

The overall structure is solid. Fix the __openapi_service naming, the wait_for_next_run return-value / clock-skew issues, and add the docs update before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant