feat: support auto-resolved template model IDs#216
Closed
xichengpro wants to merge 2 commits into
Closed
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to resolve template model IDs dynamically by querying server capabilities. It adds a utility function resolve_template_model_id and integrates it into client template setup methods, while also updating the server launcher to merge configured supported models with those derived from application configs. Feedback highlights a bug where self.model_id retains the twinkle:// prefix in MultiLoraTransformersModel, and suggests improvements such as using model_dump() or dict() for Pydantic model normalization, calling raise_for_status() before parsing JSON responses, and adding exception handling to gracefully handle unreachable servers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+57
to
+65
| if hasattr(item, 'model_name'): | ||
| model_name = getattr(item, 'model_name', None) | ||
| if not model_name: | ||
| return None | ||
| template_init_model_id = getattr(item, 'template_init_model_id', None) | ||
| return { | ||
| 'model_name': model_name, | ||
| 'template_init_model_id': template_init_model_id, | ||
| } |
Contributor
There was a problem hiding this comment.
When normalizing a supported model item, if the item is a Pydantic model (which has the model_name attribute), extracting only model_name and template_init_model_id will silently drop any other fields that might be present on the model. To ensure extensibility and preserve all fields, we should check if the item has a model_dump or dict method and use that to serialize it to a dictionary.
Suggested change
| if hasattr(item, 'model_name'): | |
| model_name = getattr(item, 'model_name', None) | |
| if not model_name: | |
| return None | |
| template_init_model_id = getattr(item, 'template_init_model_id', None) | |
| return { | |
| 'model_name': model_name, | |
| 'template_init_model_id': template_init_model_id, | |
| } | |
| if hasattr(item, 'model_name'): | |
| if hasattr(item, 'model_dump'): | |
| return item.model_dump() | |
| if hasattr(item, 'dict'): | |
| return item.dict() | |
| model_name = getattr(item, 'model_name', None) | |
| if not model_name: | |
| return None | |
| template_init_model_id = getattr(item, 'template_init_model_id', None) | |
| return { | |
| 'model_name': model_name, | |
| 'template_init_model_id': template_init_model_id, | |
| } |
Comment on lines
+14
to
+15
| response = http_get(f'{base_url}/twinkle/get_server_capabilities') | ||
| _SERVER_CAPABILITIES_CACHE[base_url] = GetServerCapabilitiesResponse(**response.json()) |
Contributor
There was a problem hiding this comment.
The HTTP response from http_get is parsed directly via response.json() without calling response.raise_for_status(). If the request fails (e.g., due to a 404 or 500 error), this can lead to unhelpful or cryptic errors (like JSONDecodeError or ValidationError). Calling raise_for_status() first ensures that HTTP errors are surfaced clearly.
Suggested change
| response = http_get(f'{base_url}/twinkle/get_server_capabilities') | |
| _SERVER_CAPABILITIES_CACHE[base_url] = GetServerCapabilitiesResponse(**response.json()) | |
| response = http_get(f'{base_url}/twinkle/get_server_capabilities') | |
| response.raise_for_status() | |
| _SERVER_CAPABILITIES_CACHE[base_url] = GetServerCapabilitiesResponse(**response.json()) |
Comment on lines
+31
to
+33
| supported_model = get_supported_model_by_name(model_name) | ||
| if supported_model and supported_model.template_init_model_id: | ||
| return supported_model.template_init_model_id |
Contributor
There was a problem hiding this comment.
If the server is temporarily unreachable or running an older version that does not support the /twinkle/get_server_capabilities endpoint, get_supported_model_by_name will raise an exception and crash the client's set_template call. Wrapping this in a try-except block ensures defensive programming and allows the client to gracefully fall back to using model_name directly.
Suggested change
| supported_model = get_supported_model_by_name(model_name) | |
| if supported_model and supported_model.template_init_model_id: | |
| return supported_model.template_init_model_id | |
| try: | |
| supported_model = get_supported_model_by_name(model_name) | |
| if supported_model and supported_model.template_init_model_id: | |
| return supported_model.template_init_model_id | |
| except Exception: | |
| pass |
Collaborator
|
Hi @xichengpro, Thanks — the capability-declaration flow is a nice cleanup, and the Heads up: #210 is a parallel server-config refactor that reshapes the launcher ( Since the launcher helpers here would need to be rewritten against the typed config, we'll fold the intent into #210 directly with |
Yunnglin
added a commit
that referenced
this pull request
Jun 5, 2026
Sampler's set_template now auto-injects self.model_id when the caller omits it, consistent with how the model backend already overrides model_id with self.tokenizer_id. This eliminates the need for clients to manually resolve the HF model ID when the route name differs from the underlying model. Also fixes two client-generator bugs: model client assigned self.model_id before stripping the twinkle:// prefix, and sampler client was missing self.model_id entirely. Co-authored-by: xichengpro <188454548+xichengpro@users.noreply.github.com>
Yunnglin
added a commit
that referenced
this pull request
Jun 11, 2026
* refactor(server): extract state module and introduce StateBackend abstraction
- Move utils/state/ to server/state/ as top-level module
- Fix all 8 import references (no re-export compatibility layer)
- Add StateBackend ABC with set/get/delete/exists/keys/count/set_nx/close/health_check
- Implement MemoryBackend (sync in-memory, compatible with Ray Actor)
- Refactor ConfigManager to use StateBackend with 'config::' key prefix
- Inject optional backend parameter into ServerState and get_server_state factory
- Add unified exception hierarchy (TwinkleServerError and subclasses)
- Create telemetry/ skeleton directory for Phase 2
* feat(server): replace Ray metrics with OpenTelemetry observability (trace/metric/log)
- Add telemetry/provider.py: OTEL TracerProvider/MeterProvider/LoggerProvider init
with debug (console) and OTLP export modes, graceful shutdown
- Add telemetry/metrics.py: MetricsRegistry singleton facade over OTEL meters
(low-invasiveness: business code uses MetricsRegistry.get() only)
- Add telemetry/tracing.py: get_tracer/inject_context/extract_context with
noop fallback when OTEL SDK is not installed
- Rewrite utils/metrics.py as thin adapter layer: _Counter/_Histogram/_Gauge
map Ray-style API (inc/set/observe) to MetricsRegistry OTEL instruments
- Update server_state.py _metrics_loop to use MetricsRegistry UpDownCounter
with delta calculation
- Inject trace context in gateway/proxy.py for distributed tracing
- All OTEL imports are guarded (optional dependency): server starts normally
without opentelemetry packages installed (NoOp fallback)
- Completely remove ray.util.metrics dependency (zero residual references)
* fix(server): remove duplicate config methods in ServerStateProxy
* feat(server): add persistence layer with FileBackend, RedisBackend, and config signature
- Implement FileBackend (JSON file storage with atomic write, fcntl lock, TTL)
- Implement RedisBackend (redis.asyncio, optional dependency with guard)
- Add PersistenceConfig + create_backend factory (memory/file/redis modes)
- Adapt all Managers (Session/Model/Sampling/Future) to use StateBackend
- BaseManager: async CRUD via backend with key prefix isolation
- ModelManager: hybrid mode (records persisted, indexes in-memory with rebuild)
- Add config signature validation (SHA256 hash, warn/clear/abort policies)
- Fix ABORT policy exception propagation in get_server_state
- Add comprehensive unit tests (62 passed)
* feat(server): integrate telemetry into Ray Serve workers and fix persistence config parsing
* fix(server): replace FastAPIInstrumentor with custom tracing middleware for Ray Serve compatibility
* update dockerfile
* fix(server): harden telemetry/persistence wiring and middleware order
- tracing middleware: return passthrough when OpenTelemetry SDK is absent
instead of crashing every request on lazy import inside the handler
- persistence_config: propagate via TWINKLE_PERSISTENCE_* env vars from the
launcher to all Ray workers, so the configured backend is used regardless
of which deployment initializes ServerState first; lift the example to
top-level YAML
- middleware order: register metrics last in all four apps so it wraps the
outermost layer and captures full end-to-end latency including tracing
- example yaml: telemetry default to enabled=false (optional dependency),
document how to opt in
* docs(observability): add LGTM-based docker-compose stack for telemetry
cookbook/observability/ provides a one-container OTLP receiver + dashboard
for Twinkle, built on grafana/otel-lgtm (bundled OTel Collector + Mimir +
Tempo + Loki + Grafana). Users docker compose up, point telemetry_config
at localhost:4317, and get a pre-provisioned overview dashboard with HTTP
rate / latency, queue depth, task latencies, rate-limit rejections, and
active-resource gauges.
* test(server): add client-API contract harness and baseline snapshot (Phase 0a)
Establishes the cross-cutting freeze guard for the Tinker/Twinkle HTTP
contract (R20, R18.1) ahead of the server-config + observability refactor.
The harness builds each FastAPI app (gateway, model, sampler, processor)
by registering its route helpers against a fresh app, then extracts the
OpenAPI paths and component schemas. The committed baseline at
tests/contract/client_api_baseline.json is what every later phase asserts
equality against to catch any drift in route paths, HTTP methods, or
request/response schemas. Adds hypothesis to the test extras for the
property-based tests later phases will need.
* refactor(server): convert TaskQueueConfig to Pydantic with field constraints (Phase 0b)
Replaces the dataclass with a Pydantic BaseModel so invalid rate-limit and
timeout values are rejected at construction instead of leaking into the
running deployment. Constrains rps_limit/tps_limit/queue_timeout/
token_cleanup_interval to >= 0, window_seconds to > 0, and max_input_tokens
to int >= 1, matching R9.2-9.5/9.7. Sets extra='forbid' so unknown YAML
keys surface immediately. The from_dict(config_dict=None) factory is
preserved for the existing call sites in model/sampler/processor apps and
now delegates to model_validate({}) when no input is given.
Adds property tests (Hypothesis, max_examples=100) for constraint
enforcement, from_dict equivalence with model_validate, and the documented
defaulting behaviour. The Phase 0a client-API contract baseline is re-run
green as the cross-cutting freeze guard.
* feat(server): introduce typed ServerConfig aggregate root (Phase 0c)
Adds a single Pydantic aggregate root that drives the launcher: ServerConfig
nests TelemetryConfig, PersistenceConfig, TaskQueueConfig, and a list of
typed ApplicationSpec entries. Each per-deployment args block has its own
schema (ModelArgs/SamplerArgs/ServerArgs/ProcessorArgs) with extra='forbid',
so unknown keys and out-of-range values surface at load time with the
offending field path. backend (model) and sampler_type (sampler) are
introduced as Literal-validated selectors, replacing the legacy use_megatron
boolean — Phase 1 will wire the actual dispatch on these values.
ServerConfig.from_yaml is the single load entry point: FileNotFoundError on
a missing path, ConfigParseError on malformed YAML, ValidationError on field
or cross-field violations. The cross-field validator rejects redis mode
without redis_url and file mode without file_path. ServerLauncher now
requires a typed ServerConfig and rejects raw dicts; from_yaml became a
thin wrapper. Legacy field names telemetry_config/persistence_config are
rejected per the breaking-change clause in R8.
Migrates the cookbook example configs (transformer + megatron) to the new
field names and adds property tests covering valid/invalid loads, round-trip
fidelity, and legacy-name rejection. The Phase 0a client-API contract
baseline is re-run green as the cross-cutting freeze guard.
Adds ConfigError (field/value/allowed) and ConfigParseError to
server.exceptions for callers that want a single non-pydantic exception
type to catch.
* refactor(server): direct-backend ServerState, drop detached actor (Phase 0d)
Removes the single detached Ray Actor that centralized server state
(get_server_state used to call ray.remote(ServerState).options(lifetime='detached'))
and replaces it with a process-local ServerState bound directly to the
configured StateBackend. Every deployment now reads and writes through the
shared backend, which removes the actor as a single-point bottleneck and
makes state visibility a property of the backend (Redis cross-process,
MemoryBackend in-process) rather than the actor.
Adds ReplicaRegistry persisting capacity at replica::<replica_id>::max_loras
so two workers on a shared backend agree on the cluster's available
LoRA capacity. ModelManager loses its in-memory _replica_max_loras /
_replica_models / _token_models dicts: capacity, per-replica loaded counts,
and per-token model counts are derived from persisted ModelRecords on each
read. register_replica / unregister_replica / get_available_replica_ids /
get_capacity_info are now async to match the backend roundtrip; ServerState
awaits them through. ServerStateProxy stays as a typing alias of ServerState
so existing call-site annotations keep working without import churn.
Updates the existing manager tests to the new async API and adds a Phase 0d
test module: a static + dynamic check that no detached actor is created
(R19.1), an in-process MemoryBackend smoke test (R19.6), the
ReplicaRegistry round-trip, cross-instance visibility on a shared
MemoryBackend, and a Hypothesis property (Property 25) showing two
ServerState instances driven by the same op stream agree on every read.
The Phase 0a client-API contract baseline is re-run green.
* feat(server): mock model + sampler backends with case-sensitive dispatch (Phase 1)
Adds numpy-only TwinkleCompatMockModel and MockSampler so the server can be
launched on a CPU-only host with no torch / transformers / vllm / megatron
installed. Both backends return deterministic results keyed by the request
parameters: forward / forward_only / forward_backward yield logprob and
elementwise_loss arrays whose shapes are derived from the input sequence
lengths, sample emits one logprob per token and num_samples sequences per
prompt, and identical requests produce identical bytes (R1.3, R2.3-2.5,
R4.4, R4.5). Adapter add / remove / has are tracked in an in-memory record;
remove on an absent name raises KeyError without mutating the record (R1.7).
Replaces the if-use_megatron branch in model/app.py with strict case-
sensitive dispatch on the new ``backend`` field (mock|transformers|
megatron) and the hardcoded vLLMSampler in sampler/app.py with dispatch on
``sampler_type`` (mock|vllm|torch). Both validators raise ConfigError with
field/value/allowed *before* instantiating any backend (R3.9, R3.10) and
the mock branch skips ``twinkle.initialize(mode='ray', ...)`` entirely
(R3.7, R3.8) — the largest startup-time saving on a CPU-only host.
Makes ``twinkle.server.model`` and ``twinkle.server.sampler`` package
__init__s lazy via __getattr__ so importing the mock backend module does
not transitively pull torch (via app.py → common/router → template) or
vllm (via app.py → twinkle.sampler) on a CPU-only host (R1.2, R2.2, R4.3).
Adds the all-mock cookbook config at cookbook/client/server/mock/ with a
README documenting the launch command, the 30-second ready-state target,
and an explicit not-for-production note. Mock-mode persistence defaults to
in-process MemoryBackend so no Redis is required.
Property tests cover interface conformance, forward determinism + shape,
adapter round-trip, remove-absent semantics, sampler output length and
logprob count, sampler determinism, max_tokens<1 rejection, and dispatch
validation for every (field, allowed, invalid) tuple. Static checks
guarantee mock_sampler.py never imports vllm directly and mock_model
imports successfully when torch/transformers/vllm/megatron are blocked
from sys.modules. The Phase 0a client-API contract baseline is re-run green.
* feat(server): business-layer tracing + correlation + resource metrics (Phase 2)
Adds a traced_operation context manager that wraps a business-layer block
in one OpenTelemetry span: starts before the block, records exceptions and
sets span status to ERROR on raise, ends after the block, and re-raises the
original exception (R10.1, R10.4). The helper degrades to a NoOp context
manager when the OTEL SDK is missing so call sites get the same return
value with or without tracing installed (R10.5 / R18.3).
Defines the standardized correlation keys (twinkle.session_id /
twinkle.model_id / twinkle.replica_id / twinkle.token_id /
twinkle.sampling_session_id / twinkle.base_model) in a new
telemetry/correlation.py and adds set_correlation_attrs(span, values) which
attaches only present (non-None) values so partially-known operations
never end up with empty attributes (R11.1, R11.2, R11.3).
Wraps every server-state mutation that creates / registers an entity —
create_session, register_model, register_replica, create_sampling_session
— with traced_operation and the matching correlation attributes.
Adds ResourceMetricsCollector exposing observable gauges for system CPU
utilization, system memory, process RSS memory, and per-GPU utilization /
memory (R12.1). The collector is started by ensure_telemetry_initialized
in each Ray Serve worker, including when telemetry is disabled, so the
graceful-degradation path matches the enabled path (R12.2). When psutil
or pynvml is missing, or no GPU is present, the affected gauges report no
data and the collector does not raise (R12.3 / R18.3). Declares psutil
and pynvml as a new [telemetry] extras group in pyproject.toml (R12.4).
Property tests (Hypothesis, max_examples=100) cover the prefix invariant,
correlation attachment skipping None, and NoOp degradation equivalence;
unit tests verify span lifecycle and exception recording against an in-
memory OTEL exporter, and the wiring tests confirm the worker-init hook
calls into the collector regardless of TWINKLE_TELEMETRY_ENABLED. The
Phase 0a client-API contract baseline is re-run green.
Note: the Grafana dashboard CPU/Mem/GPU panels (task 7.13) and the LGTM
integration tests (7.15) require the docker-compose stack and are deferred
to the documentation phase.
* feat(server): typer CLI with launch-time config-drift validation (Phase 3)
Replaces the argparse __main__ with a typer-based operations CLI living in
twinkle.server.cli. The CLI exposes four subcommands:
- launch — start the server from a YAML config. Validates the
persistence config signature against the persistence
backend BEFORE ray.init so a configuration drift fails
fast (R15.1).
- check-config — exit 0 on a valid config, non-zero with the offending
field/error on failure (R14.3, R14.4).
- print-config — emit the validated, normalized ServerConfig as YAML or
JSON; the JSON output round-trips back to an equal
ServerConfig (R14.5).
- clear persistence — delete persisted state for the namespace derived
from a config (R14.2).
Every option declares envvar= so env vars apply when the flag is omitted
(R14.6). The new console script twinkle-server is registered under
[project.scripts] and python -m twinkle.server delegates to the same
typer entry point, so the documented launch path is one shim layer.
Adds validate_against_backend in state/config_signature.py: builds the
backend from a PersistenceConfig, computes the current signature, stores
it on first run, and on mismatch raises ConfigMismatchError with a
stored-vs-current diff and a remediation hint pointing at the
clear-persistence subcommand (R15.2, R15.3, R15.4).
Adds a fully documented example config at
cookbook/client/server/server_config.example.yaml — every field carries
its type, default, and available options. Loadable as-is via check-config.
CLI tests cover subcommand existence, exit-code semantics, env-var
override, print-config round-trip, the order-of-operations property
(launch validates drift BEFORE ServerLauncher is even imported), and the
drift detection / first-run-storage property (Property 29). The Phase 0a
client-API contract baseline is re-run green.
* feat(server): trace context carrier for cross-deployment propagation (Phase 4)
Adds make_carrier() and activate_carrier(carrier) in
telemetry/context_carrier.py so internal Ray Serve DeploymentHandle calls
can keep one trace continuous: the calling deployment serializes its
active OTEL context into a small dict, the receiving deployment wraps its
handler body in activate_carrier(...) and any spans it starts attach as
children of the propagated context.
When the OTEL SDK is missing, make_carrier returns an empty dict and
activate_carrier becomes a no-op context manager, so the body always
runs and never raises (R13.4 / R18.3). When the carrier is None or
empty, activate_carrier also degrades to a no-op so the receiving side
just starts a fresh trace.
Adds Property 24 round-trip tests against an in-memory OTEL exporter
showing parent.trace_id == child.trace_id when the carrier is honored,
and that both sides are safe in the absence of OTEL or context. Refactors
the telemetry test fixture into a session-scoped conftest because OTel's
trace.set_tracer_provider is one-shot per process — the second per-module
fixture would have silently shared the first one's exporter and made
tests order-dependent. The Phase 0a client-API contract baseline is
re-run green.
Note: the LGTM single-trace-id fan-out integration test (task 10.4)
requires the docker-compose stack and is deferred to the documentation
phase.
* docs(server): observability + server-configuration guides (Phase 5)
Adds the documentation set the refactor has been building toward:
- docs/source_en/Usage Guide/Observability.md +
docs/source_zh/使用指引/可观测化.md
Document the six twinkle.* correlation keys, the make_carrier /
activate_carrier mechanism for cross-deployment trace propagation, and
an end-to-end LGTM example using the cookbook/observability/
docker-compose stack (R17.1, R17.2, R11.4).
- docs/source_zh/使用指引/服务配置.md
ServerConfig field reference (every top-level + applications args
schema), the supported environment variables (TWINKLE_SERVER_CONFIG,
TWINKLE_RAY_NAMESPACE, telemetry / persistence env-var bag), a minimal
YAML example, and a legacy → current field migration table covering
telemetry_config → telemetry, persistence_config → persistence, and
use_megatron → backend (R17.3, R8.3).
Adds index links to both guides from docs/source_zh/index.rst and the
Observability guide from docs/source_en/index.rst (R17.4).
Adds tests/docs/test_docs_smoke.py asserting every required content
element is present: all six correlation keys appear in both observability
guides, the propagation section names DeploymentHandle / make_carrier /
activate_carrier, the LGTM example references the docker-compose stack,
the config guide lists every top-level field + the env vars + the YAML
example + the migration table, and the index entries resolve.
The Phase 0a client-API contract baseline is re-run green, and all 210
unit + property + contract tests pass in the twinkle conda env.
* fix(server): address self-review findings (Phase 0a–5)
Cleans up the bugs / dead code surfaced by the post-implementation review:
1. Cleanup task scheduling — removes the asyncio.get_running_loop() hack
in get_server_state(): every Ray Serve worker's FastAPI lifespan now
awaits state.start_cleanup_task() explicitly. Resource expiry actually
runs again (previously every call site was sync ctor → no loop → loop
never started). Wired in gateway/model/sampler/processor lifespans;
start_cleanup_task is idempotent so repeat calls are no-ops.
2. ApplicationSpec — model/sampler entries with no args block now raise
with the offending field path instead of silently substituting a
ServerArgs() default. The mode='before' validator routes the raw args
(or {}) through the schema selected by import_path so missing required
fields surface cleanly. ApplicationSpec.args lost its silent default;
server/processor (whose schemas are all-optional) still accept bare
entries.
3. Grafana dashboard (R12.5) — adds CPU utilization, system + process
memory, GPU utilization, and GPU memory panels to twinkle-overview.json
wired to the metric names the ResourceMetricsCollector exports. Adds a
regression test covering the panel titles and target metric names.
4. Nested extra='forbid' — TelemetryConfig and PersistenceConfig now
reject unknown keys, so typos inside `telemetry: {...}` /
`persistence: {...}` fail at load time instead of silently being
dropped. Adds a parametrized regression test.
5. Validation before side effects (R3.9, R3.10) — splits each dispatch
into _validate_* (pure, no imports) and _dispatch_* (assumes validated
input). Both ModelManagement.__init__ / SamplerManagement.__init__
and the build_*_app entry points call _validate_* up front, so an
invalid backend / sampler_type never reaches twinkle.initialize,
DeviceGroup construction, or any backend import.
6. Dead code — drops the unused _BACKEND_VALUES / _SAMPLER_TYPE_VALUES
constants in application_spec.py and the dead exception branch around
the old loop.create_task call in get_server_state.
7. use_megatron legacy bridge — removed from ModelManagement.__init__,
build_model_app, and the .bind() call. backend is the canonical
selector; the only remaining mention in repo lives in the tasks-doc
migration table.
9. Stale ServerState docstring — updated to reflect direct-backend access.
11. launcher.py — single top-level `import os` instead of four duplicated
local imports.
Test surface goes 210 → 213 (added: nested-config extras, dashboard
panels, refactored validation tests). All 213 unit + property + contract
tests pass and 11 end-to-end smoke checks (cookbook YAMLs, CLI exit
codes, print-config round-trip, mock determinism, dispatch validation,
contract baseline, cleanup-task lifecycle, ApplicationSpec strictness,
nested-config strictness) pass clean.
* test(server): Docker-backed integration tests (Phase 0d/3/4/5)
Adds the integration tests previously deferred behind "needs Docker":
- tests/server/state/test_redis_integration.py — Property 26 / 27 against
a real Redis (R19.4 / R19.5). Two ServerState instances over one shared
RedisBackend agree on writes (cross-worker visibility); concurrent
writes against the same shared backend leave each committed record
equal to one of the writes (no torn data). Skips when REDIS_URL is
unreachable.
- tests/server/cli/test_drift_integration.py — end-to-end Phase 3 drift
validation against Redis (R15). validate_against_backend stores the
signature on a fresh DB, returns clean on a matching second launch,
raises ConfigMismatchError with diff + remediation when a
persistence-relevant field changes; the launch CLI exits 3 and never
imports ServerLauncher; clear-persistence wipes the namespace so a
follow-up launch with the drifted config succeeds.
- tests/integration/test_mock_mode_startup.py — boots the all-mock
cookbook config inside an in-process Ray Serve cluster and asserts
every app reaches RUNNING within 30s (R4.1, R4.2). Gated behind
TWINKLE_TEST_INTEGRATION=1 so plain pytest stays fast.
- tests/integration/test_lgtm_telemetry.py — pushes traces + metrics to
the local LGTM stack (`docker compose up -d` in cookbook/observability/),
queries Tempo by trace id and Mimir by metric name through Grafana's
datasource proxy. Confirms business spans carry twinkle.session_id /
twinkle.model_id (R11.2), the resource collector's CPU/memory gauges
show up in Mimir (R12.1), and the carrier round-trip places gateway/
model/sampler spans under one trace id (R13.3). Skips when the OTLP
endpoint and Grafana aren't reachable.
Tasks 4.7 / 4.8 / 6.19 / 9.6 marked complete in tasks.md. Tasks 7.15
and 10.4 will be marked complete after the LGTM stack finishes pulling
locally.
* test(server): OTLP trace integration test runs against Jaeger fallback
The grafana/otel-lgtm:latest image is ~3GB and proved too slow to pull
reliably on the local network. Restructures the LGTM test to auto-detect
which trace backend is up:
- Tempo via Grafana (preferred) — bundled docker-compose stack
- Jaeger 1.62.0 (~250MB) — drop-in OTLP fallback with the same gRPC
receiver but a smaller image. `docker run -d -e COLLECTOR_OTLP_ENABLED=true
-p 16686:16686 -p 4317:4317 jaegertracing/all-in-one:1.62.0`
Either backend hosts the same e2e proof: a span with twinkle.session_id /
twinkle.model_id round-trips through the OTLP pipeline (R11.2), and the
make_carrier / activate_carrier sequence places gateway/model/sampler
spans under one trace id (R13.3).
Resolves a test-isolation bug: tests/server/telemetry/conftest.py
installs an InMemorySpanExporter via trace.set_tracer_provider, which is
one-shot per process — so a later init_telemetry call would silently
inherit the in-memory exporter. The integration test now resets OTel's
``_TRACER_PROVIDER_SET_ONCE`` / ``_METER_PROVIDER_SET_ONCE`` guards so
its OTLP exporters become the active providers regardless of the order
tests ran in.
R12.1 (resource gauges expose) and R12.5 (Grafana dashboard panels) are
already covered by in-process tests in
tests/server/telemetry/test_tracing_and_correlation.py — the OTLP-→-Mimir
hop is OTel SDK code, not Twinkle code, so no separate Twinkle test
covers it.
Marks tasks 7.15 and 10.4 complete in tasks.md. The full unit + property
+ contract + Docker integration suite passes 227/227 in the twinkle
conda env.
* fix(server): bind OTLP LoggingHandler to twinkle logger so server logs reach OTLP
twinkle.utils.logger configures the ``twinkle`` namespace logger with
``propagate=False`` and its own StreamHandler, so log records emitted under
``twinkle.*`` (which is the entire server codebase) never bubble up to root.
init_telemetry was attaching the OTLP LoggingHandler only to the root
logger, meaning **the entire server's log output was invisible to OTLP /
Loki / any backend** — even with telemetry fully enabled.
Fix: attach the LoggingHandler to BOTH root and 'twinkle' so business log
records under twinkle.server.*, twinkle.demo, etc. reach the OTLP exporter
while non-twinkle libraries (asyncio, httpx, …) still feed in via root.
shutdown_telemetry detaches from both.
Verified by emitting 88 log records under twinkle.demo and confirming all
88 land in the local LGTM stack's Loki. The records carry trace_id /
span_id / severity_text as OTel structured metadata, so in Loki you can
filter with ``{service_name="twinkle-server"} | trace_id = \`<id>\``` to
pull every log for one trace.
Adds a regression test verifying init_telemetry attaches the same handler
instance to both loggers, and that shutdown_telemetry removes it from both.
* feat(observability): multi-user SFT demo + declare redis as optional extra
- pyproject.toml: add `redis = ["redis>=5.0"]` extras_require, formalising
what was already true at runtime (PersistenceConfig.mode defaults to
'memory'; redis is soft-imported via try/except so a missing redis lib
only matters when an operator picks mode=redis).
- cookbook/observability/demo_sft_users.py: scripted end-to-end SFT demo
for the LGTM stack. Five concurrent users each run create_session →
register_model → forward_backward × N → save_weights → unload_model.
Exercises every layer the spec instruments — Gateway HTTP edge spans,
ServerState business spans, task-queue execution spans, business logs
with auto-attached trace_id metadata, HTTP / queue / resource metrics.
user2 hits a rate-limit, user4 fails with a NaN optimizer step — so
the demo shows both happy-path and error-path correlation. Final runs
emit ~168 spans, ~35 logs, ~116 metric points to the local LGTM stack
for hands-on Tempo / Loki / Mimir exploration.
* style: pre-commit pass — flake8 / isort / yapf / pyupgrade / quote fixes
Runs the project's pre-commit hooks across every file touched by this
branch, so the lint CI job passes:
- flake8: wrap a handful of >120-char lines (mostly docstrings); drop the
unused ``payload``/``backend`` locals in two tests; move the
``from twinkle...`` import after ``pytest.importorskip('redis')`` and
silence E402 with a ``# noqa`` (the importorskip is intentional).
- isort: reorder imports to PyCQA's canonical layout.
- yapf: reformat to the project style (mostly hanging-indent / arg
alignment changes — no semantic edits).
- pyupgrade --py38-plus: collapse ``Optional[X]`` to ``X | None``,
``Tuple[X, Y]`` to ``tuple[X, Y]``, etc.
- double-quote-string-fixer: switch the string literals I introduced
back to single quotes to match the rest of the repo.
No behavior change. 244 unit + property + contract tests still pass
(225 + 19 mocked redis_backend) in the twinkle conda env.
* chore: gitignore .kiro/ (local spec/planning notes)
* style: convert double-quoted f-strings to single quotes (CI Python 3.11)
The pre-commit-hooks v6.0.0 ``double-quote-string-fixer`` skips
``FSTRING_*`` tokens on Python 3.12+ but on the CI runner's Python 3.11
the f-strings are emitted as a single ``STRING`` token and get rewritten.
Manually converted the 4 affected files so the hook is a no-op on either
interpreter:
- src/twinkle/server/state/base.py
- src/twinkle/server/state/backend/redis_backend.py
- src/twinkle/server/state/config_signature.py
- tests/server/state/test_managers.py
Verified: pre-commit run --all-files passes under a fresh Python 3.11.15
env (CI's runner version), and 244 unit + property + contract tests still
pass under the twinkle env (Python 3.12).
* fix(server): address code-review gaps from server-config-observability-refactor
- worker.py: emit `task_queue.execute` (R10.2) + nested `<deployment>.<task_type>`
(R10.3) spans so the queued handler op is observable, not just state-level ops.
- sampler/processor non-queued handlers: wrap primary ops in `traced_operation`
so set_template / add_adapter / apply_patch / processor.create / processor.call
also satisfy R10.3.
- config_signature: persist `_meta::config_payload` on first run so drift diff
renders real stored-vs-current field differences (R15.3) instead of always
showing the current config as if it were entirely new.
- mock model / sampler: replace Python's salted `hash(tuple-of-strings)` with
SHA-256 over a canonical string form so deterministic outputs (R2.5/R4.4/R4.5)
hold across processes — built-in hash is PYTHONHASHSEED-salted and would
diverge across replicas / restarts.
- context_carrier: document that the current topology routes every cross-
deployment hop through the Gateway HTTP proxy (already trace-propagating),
so there are no in-process DeploymentHandle call sites to thread the carrier
through today; the helpers remain the supported integration point for any
future handle-based hop.
- launcher: wire ServerConfig.proxy_location into `serve.start(...)` (example
configs already declare it) and make ApplicationSpec a real top-level import
so `get_type_hints(_deploy_application)` resolves at runtime.
* refactor(server): drop ServerStateProxy alias, use ServerState directly
The proxy class was removed in Phase 0d (de-Actor); only the
`ServerStateProxy = ServerState` alias survived so existing type hints could
keep working through the transition (R19.1). With every call site updated,
the alias is now misleading — there is no proxy, just direct backend access.
- Delete the alias and its retention comment in `state/server_state.py`.
- Remove the re-export from `state/__init__.py`.
- Rename all 7 call-site type hints (`router`, `lifecycle/base`,
`task_queue/mixin`, `task_queue/worker`, `model/app`, `sampler/app`,
`processor/app`) to `ServerState`.
Pure rename — zero behavior change. The Client_Facing_API contract is
unaffected (R20).
* docs(observability): add load.py to populate every Grafana overview panel
Mock-mode servers leave most dashboard panels at "No data" because:
- `histogram_quantile(..., rate(_bucket[5m]))` returns NaN under zero recent
traffic — sparse requests render counters but blank histograms;
- `up_down_counter` gauges (`active_sessions`, `active_models`, `queue_depth`)
emit on delta only and stay invisible until the underlying count moves;
- mock backends execute in microseconds so P95 hugs the bottom bucket.
`load.py` drives a running mock server with N concurrent users that each:
POST /api/v1/twinkle/create_session -> active_sessions++
POST /api/v1/model/mock/twinkle/add_adapter_to_model -> active_models++
POST /api/v1/sampler/mock/twinkle/sample (loop, 80%) -> http rate + latency
+ queue_depth
+ task_execution
+ task_wait
POST /api/v1/model/mock/twinkle/forward_only (~20%) -> sticky-LoRA path
Sticky `X-Ray-Serve-Request-Id` is pinned per (user, adapter) so the
`request_id + '-' + adapter_name` lookup in `assert_resource_exists` resolves
on subsequent /forward_only calls.
Tunable: `--concurrency`, `--duration`, `--interval`, `--max-tokens`. Bumping
`--max-tokens` lifts mock execution time off the bottom histogram bucket so
P95 panels show meaningful values.
* fix(server): MockSampler accepts handler kwargs; load.py uses TELEMETRY_ENABLED=1
Two issues exposed when actually running cookbook/observability/load.py
against a live mock server:
1. ``MockSampler.sample(...)`` raised ``TypeError`` because the
Tinker / Twinkle handlers forward ``adapter_path`` (matching the
``vLLMSampler`` signature) but the mock didn't accept extra kwargs.
Added ``**kwargs`` so the mock stays callable through the same handler
call sites — 100% of ``/sample`` requests now succeed.
2. load.py docstring told users ``TWINKLE_TELEMETRY_ENABLED=true`` but
``worker_init.ensure_telemetry_initialized`` reads the literal ``"1"``,
so telemetry was never actually initialised — every panel showed "No
data". Corrected to ``=1`` and added the ``ray start --head`` prereq
(the launcher does ``ray.init(address='auto')`` and won't bootstrap one).
* fix(server): lazy-start ServerState cleanup loop on first request
Ray Serve binds ``serve.get_replica_context().servable_object`` AFTER
FastAPI ``lifespan`` startup completes, so the existing lifespan call to
``get_self().state.start_cleanup_task()`` crashed with
``'NoneType' object has no attribute 'state'`` in every worker and was
silently swallowed. The cleanup loop drives ``_metrics_loop``, which
emits the four ``twinkle_*_active`` resource gauges — so those gauges
never produced a single sample and the "Active resources" Grafana panels
always read "No data".
Move the call from lifespan to first-request lazy-init:
- Model / Sampler: ``_on_request_start`` -> ``_ensure_state_cleanup_started``
- Processor: ``_ensure_sticky`` (which every routed call goes through)
- Gateway: a tiny ``ensure_state_cleanup_started`` HTTP middleware (no
per-handler hook exists)
``state.start_cleanup_task`` is itself idempotent via ``_cleanup_running``;
the per-instance flag avoids the await call on every subsequent request.
Verified end-to-end against a live mock server with the LGTM stack:
``twinkle_sessions_active=4`` and ``twinkle_futures_active`` now emit
correctly. (``twinkle_models_active`` still empty under load — separate
diagnostic for a follow-up; ``models.add`` reaches the backend and
``futures.add`` works through the same metrics loop, so likely an
instrument-binding issue at lazy-init time worth its own investigation.)
* docs(observability): load.py uses server-issued session_id, requires redis backend
Three bugs uncovered while making active-resource panels light up:
1. ``X-Twinkle-Session-Id`` used a client-side string that the server never
persisted, so the adapter countdown loop in ``utils/lifecycle/base.py``
saw "session not found" and expired every registered adapter within ~10s.
Now call ``/twinkle/create_session``, take the server-issued id from the
response body, and pass that id to every subsequent header. Also heartbeat
the session every 5s so it stays alive.
2. ``persistence: memory`` is per-process — Gateway-worker sessions are
invisible to the Model worker. Even with the correct session_id the
liveness check still fails because Model's MemoryBackend has zero session
records. Docstring now states the script requires a shared backend
(Redis) and explains the trap; reasonable, since R19.4 cross-worker
visibility specifically requires shared persistence.
3. Sampling-session calls hit ``/api/v1/twinkle/create_sampling_session``
and 404'd because the route lives at the gateway root (it is a Tinker
route — only ``create_session`` is mounted under ``/twinkle/``). Fixed
to call ``/api/v1/create_sampling_session``.
Result against a redis-backed mock server: 13/14 dashboard panels populate
(``rate_limit_rejections`` stays empty under gentle load — by design;
``gpu_*`` stays empty on a CPU-only mock — by design).
* chore: drop unfinalized mock/observability surface from docs + cookbook
Mock backends and OpenTelemetry pipeline still live in src/ but their
public contract isn't settled. Pull them out of user-facing docs and
cookbook examples so iteration doesn't churn published surface.
- delete Observability.md (en/zh), strip telemetry rows + mock mentions
from 服务配置.md, drop entries from both index.rst toctrees
- delete cookbook/observability/, cookbook/client/server/mock/, and
cookbook/client/server/server_config.example.yaml
- strip telemetry: blocks from transformer/megatron server_config.yaml
- migrate the mock CPU-only YAML the e2e test needed into
tests/server/fixtures/server_config_mock.yaml; CLI + mock-mode-startup
tests import the shared path constant
- drop now-dead tests: tests/docs/test_docs_smoke.py (asserted removed
files), tests/integration/test_lgtm_telemetry.py (gated on removed
docker-compose; in-process equivalents already covered), grafana
dashboard panel test, and the two mock cookbook README/config asserts
in test_mock_sampler.py
* docs(zh): drop 服务配置.md too — same unfinalized refactor
Mirrors the en side, which has no Server-Configuration guide. The
ServerConfig schema is part of the same not-yet-confirmed surface as
mock / observability — pull it out of the published toctree now.
* fix(server): make tinker e2e go green on mock backends
Drives the upstream tinker SDK end-to-end through the all-mock cookbook
(twinkle SDK + tinker SDK over the same MockModel/MockSampler), which
surfaced four real server bugs that bit any backend (not just mock):
- gateway/retrieve_future: when ``record is None`` (cross-replica write
not yet visible) the handler short-circuited ``try_again`` instantly,
so the SDK's polling loop hammered the gateway at ~150 Hz. Fold the
None case into the long-poll budget with ``asyncio.sleep``.
- model/save_weights_for_sampler: returned ``path=None``; tinker SDK's
``_save_weights_for_sampler_async`` asserts ``result.path is not None``.
Return ``tinker_path`` (symmetric with ``save_weights``).
- sampler/asample (tinker handler): twinkle's ``SampledSequence.logprobs``
is ``List[List[Tuple[int, float]]]`` (top-k per position); tinker
expects ``Optional[List[float]]`` (chosen-token logprob). Flatten
before serializing.
- model/app: ``data_world_size`` blew up on mock backend because
``device_mesh = None``. Add a property that falls back to 1.
Plus header propagation needed for Ray Serve 2.55+ — request_id and
multiplexed-model-id are now ``x-request-id`` / ``serve_multiplexed_model_id``
(constants from ``ray/serve/_private/constants.py``). Send both new and
legacy header names from the proxy + clients to keep Twinkle's own
``verify_request_token`` middleware (which reads the legacy names by
hand) and Ray Serve both happy.
Mock backend fills:
- mock_sampler: add ``set_template`` / ``reset_prefix_cache`` (called
by the tinker sampler handler).
- mock_model: ``upload_to_hub`` no-op; ``_to_tinker_loss_outputs``
wraps numpy lists into ``tinker.types.TensorData`` (lazy import) so
``ForwardBackwardOutput.loss_fn_outputs`` validates.
- twinkle_client.sampler.vLLMSampler: stop subclassing
``twinkle.sampler.base.Sampler``; that base's package init pulls
``VLLMEngine`` → torch + zmq, defeating CPU-only client environments.
Test infra:
- ``tests/integration/test_mock_mode_startup.py`` now drives the full
/twinkle/* surface AND the upstream tinker SDK against the same mock
cluster. Tinker portion gated on ``TWINKLE_TEST_TINKER=1``.
- ``ray_cluster`` fixture mirrors ``twinkle.server.launcher``'s
``TWINKLE_PERSISTENCE_*`` env propagation into both ``os.environ``
and ``ray.init(runtime_env=...)`` — without it each Ray Serve replica
defaults to ``MemoryBackend`` and the tinker future flow can't
resolve cross-replica.
- Two persistence fixtures: ``server_config_mock.yaml`` (file-backed,
default — no external deps) and ``server_config_mock_redis.yaml``
(redis-backed, opt-in via ``TWINKLE_TEST_REDIS_PERSISTENCE=1``).
Comment cleanup: dropped internal R1.x / R2.x / R4.x requirement-id
refs from the mock backends and their tests; reviewers couldn't decode
them. Tightened a few verbose WHAT-explaining blocks to one-line WHY.
* fix(server): MemoryBackend wraps detached Ray actor for shared state
R19 made each Ray Serve worker bind its own ServerState directly to a
process-local StateBackend. With the in-process dict MemoryBackend that
silently split state across the gateway / model / processor deployments —
each got its own empty dict and the sessions / replicas / configs
registered on one worker were invisible to the others. The
``replica_registry`` comment that called MemoryBackend "single-node"
was outdated the moment R19 landed.
MemoryBackend is now a thin wrapper around a detached ``@ray.remote``
``_StateActor`` that holds the canonical dict (fnmatch + TTL semantics
unchanged). Every method forwards via ``await self._actor.X.remote(...)``,
so all workers in the cluster share one consistent store. The actor name
is namespaced by ``key_prefix`` (``twinkle_state_actor`` or
``twinkle_state_actor::<prefix>``) so multiple Twinkle deployments in
one Ray cluster do not collide. Memory mode now requires an initialized
Ray runtime; calling ``MemoryBackend()`` without one raises
``RuntimeError`` instead of falling back to the broken per-process dict.
``factory.py`` imports MemoryBackend lazily inside the ``case 'memory'``
arm so file/redis-only callers still avoid loading ray on import.
End-to-end verified on twinkle-A100-8GPU-2 with ``persistence.mode=memory``:
- 10-step self_cognition LoRA loss matches the file-mode baseline within
0.01 at every step (2.79 / 2.97 / 2.94 / 2.29 / 2.38).
- ``ray list actors`` shows ``twinkle_state_actor`` ALIVE while the
server runs, remains ALIVE (same PID) after the server is killed,
and a freshly-started server reads the prior session row through the
same actor — confirming detached lifetime + cross-process sharing.
Redis concurrency audit (new tests in ``test_redis_integration.py``,
run against a real ``redis:7-alpine`` container):
- ``SessionManager.touch`` RMW is benign for the "touch never returns
False / final heartbeat ≈ now" contract — confirmed via a 200-call
hammer test, kept as a regression.
- ``ConfigManager.add_or_get`` is safe because Redis ``SETNX`` is
atomic — covered by a new race test.
- ``FutureManager.store_status`` RMW is NOT safe: a concurrent stale
``pending`` retry can clobber a freshly committed terminal
``completed``. Test is marked ``xfail(strict=False)`` so the race
stays visible; fixing it needs WATCH/MULTI/EXEC or a Lua script and
is left to a follow-up PR. ``RedisBackend.keys()`` still uses
``KEYS pattern`` (production hazard on large key spaces); switching
to SCAN is also a follow-up.
Test infra:
- New ``tests/server/conftest.py`` boots a session-scoped Ray runtime
(``num_cpus=2, namespace='twinkle_test'``) so any ``MemoryBackend()``
in the suite works, and clears the canonical actor's store before
each test function for isolation. Hypothesis property tests reset
per-example with ``await backend.close()`` (the actor's ``close``
flushes the store).
- ``test_de_actor.py`` renamed to ``test_persistence_topology.py`` and
the two "must not contain ``ray.remote``" inverse assertions deleted
(they were correct under R19, wrong now).
- ``test_factory.py`` gains three cases: actor resolvable by name,
``key_prefix`` namespacing, and ``RuntimeError`` when Ray is down.
Test layout: ``tests/contract`` and ``tests/integration`` moved under
``tests/server`` to keep the tree flat (per project convention:
``tests/<area>/...``). Import paths and the
``python -m tests.server.contract.update_baseline`` command line
updated. No behavioural change to those suites.
* refactor(server): leader-elected cleanup + atomic state updates + LGTM stack
One PR, twelve follow-ups from the MemoryBackend revert audit:
Atomicity primitive
- StateBackend.update_atomic(key, transform, ttl) on every backend; the
Redis impl uses WATCH/MULTI/EXEC with 16-retry jittered exponential
backoff (livelock-safe) and surfaces ConcurrencyError on exhaustion.
- StateBackend.set_nx now takes ttl across backends so leader election can
hold a bounded lease.
- FileBackend funnels every public op through one fcntl.LOCK_EX critical
section — set/delete/get/keys/set_nx/update_atomic are now mutually
serialized (no more set-vs-update_atomic race).
- RedisBackend.keys() switched from KEYS to scan_iter for non-blocking
enumeration.
- MemoryBackend.health_check catches RayActorError and returns False
instead of silently re-creating the actor (which would drop all state).
Manager-level RMW races fixed
- FutureManager.store_status uses update_atomic with a do-not-regress
transform — a stale 'pending' retry can no longer clobber a freshly
committed terminal status.
- SessionManager.touch uses update_atomic; catches ConcurrencyError as
best-effort (next heartbeat retries).
Cleanup leader election + ObservableGauge metrics
- Per-backend cleanup-leader lease (30s TTL, renewed every 10s) elected
via set_nx + update_atomic on the shared StateBackend. Only the leader
runs cleanup + publishes resource counts, so the four Ray Serve
workers no longer 4x-inflate active_sessions / models / sampling /
futures.
- MetricsRegistry's four resource counters become OTEL ObservableGauges
backed by a push cache; the leader's _metrics_publish_loop updates the
cache and the gauge callbacks read it on every export tick.
- Leader hand-off intentionally does NOT zero the cache — the next leader
overwrites within metrics_update_interval, avoiding a fake Prometheus
dip.
LGTM observability stack restored
- cookbook/observability/{docker-compose.yaml, README.md, grafana/
dashboards/twinkle-overview.json} brought back, dashboard panels read
the gauges directly (no rate() wrappers needed under ObservableGauge).
- tests/server/integration/test_lgtm_telemetry.py upgraded with anti-
inflation (Mimir reads 5 not 20), trace-correlation round-trip, and a
dashboard-panel-query parametrize so PromQL drift fails the test.
Hygiene
- Cookbook server_config.yaml comments mention that memory mode requires
an initialized Ray runtime.
- Codebase-wide scrub of '(Rxx)' / 'Phase X' roadmap citations (60+
occurrences across src + tests); new pre-commit hook 'no-roadmap-refs'
rejects regressions (excluding olympiad_bench.py's algorithmic phases).
- test_create_backend_memory_mode_requires_ray_initialized moved to its
own file with private fixture so it no longer shreds the session-
scoped Ray runtime for other tests.
Verified
- 237 passed, 20 skipped on mock backend
- 24 passed on docker Redis (test_redis_integration + test_update_atomic
+ test_redis_scan_migration), including the previously-xfailed
test_concurrent_future_update_no_state_regression
- 4-instance smoke confirms exactly 1 leader and correct absolute counts
in the ObservableGauges
- pre-commit (flake8/isort/yapf/pyupgrade/no-roadmap-refs) green on all
47 PR-scope files
* track 1
* refactor(server): structural refactor + corrective Block C + review fixes
Bundles the previously-uncommitted structural refactor (Tasks 11-24), the
corrective Block C work (Tasks 25-34) from the server-structural-refactor
spec, and the follow-up fixes from this round of code review. Commit is
large because the working tree had accumulated all three.
## Structural track (Tasks 11-24)
- top-level `server/app_scaffold.py` with Shared_App_Scaffold +
LazyCleanupMixin; all four App_Builders adopt it
- `utils/backend_dispatch.py` with `BackendSelector` (MODEL_SELECTOR /
SAMPLER_SELECTOR adopted in model + sampler)
- `utils/metrics.py` → `utils/metrics_middleware.py` (disambiguate from
`telemetry/metrics.py:MetricsRegistry`)
- `gateway/server.py` → `gateway/app.py`; `build_server_app` →
`build_gateway_app`; `gateway/{tinker,twinkle}_gateway_handlers.py` →
`gateway/{tinker,twinkle}_handlers.py`
- `utils/checkpoint_base.py` + `common/{tinker,twinkle}_checkpoint.py` +
`common/checkpoint_factory.py` → `server/checkpoint/` (`base/` split
into `{models,paths,training_run_manager,checkpoint_manager}.py`)
- `launcher.py` → `launcher/` package (thin `__init__` aggregator +
`server_launcher.py` + `env_propagation.py` + `builder_registry.py`)
## Corrective Block C (Tasks 25-34)
- collapse launch entrypoints: drop `ServerLauncher.from_yaml`;
`launch_server` is the single path
- delete dead `telemetry/context_carrier.py` + its test
- mock backend kwargs hygiene (`MockSampler.__init__`,
`TwinkleCompatMockModel.__init__` accept and DEBUG-log unknown kwargs)
- typed `ServerStateArgs` replaces the untyped `server_config: dict`
- rename `MemoryBackend` → `RayActorBackend` across src + tests
- CLI: narrow config-load `except` to `pydantic.ValidationError`;
validate `--format` against {yaml, json}
- contract baseline slim (per path/method: operationId, parameters,
response status codes only; full components.schemas dropped)
## Review-driven fixes (this round)
- drop `TaskQueueConfig.from_dict` shim + `mixin.py` dict branch + dict
annotations on model/sampler `__init__` + `test_config.py` cases that
pinned the shim alive
- drop `**kwargs` from `ServerState.__init__` and `get_server_state`
(typed end-to-end; misspelled keys now fail at the boundary)
- characterization test: assert metrics → tracing → auth middleware
ordering (was only auth presence before)
- new scaffold lifespan-shutdown end-to-end test (covers the path that
the worker-side telemetry flush actually runs through, not just the
helper)
- drop dead outer try/except around scaffold cleanup middleware (mixin
already swallows)
- drop unused `ServerLauncher._BUILDERS` back-compat alias
- fix stale `test_lgtm_telemetry` docstring + cookbook YAML comment
## Test infrastructure
- characterization tests for all four App_Builders
- Hypothesis property tests for `LazyCleanupMixin` /
`BackendSelector` (Properties 1/2/3)
- import tests for `checkpoint/` + `launcher/` relocations (assert no
shim left behind)
- corrective regression tests for Tasks 4-9: cascade-cleanup TOCTOU,
per-token model-limit race, leader-recovery on renewal failure,
resource-gauge handover, telemetry-flush on shutdown, OTLP logging
feedback-loop filter
- new `test_full_sft_grafana_panels`: boots Ray + LGTM together, runs
full Twinkle SDK SFT smoke (forward/backward/step/save/load/resume/
sample/upload), asserts 9 of 12 dashboard panels return non-empty
data from Mimir within deadline
- integration test fixture fixes: pin `ray_actor_options.num_cpus` low
so 3 deployments fit on a 2-4 CPU local Ray cluster; `_query_mimir_instant`
gains `require_data=True` to actually poll until Mimir has the
datapoint (was returning on first 200-OK); reorder `test_lgtm_telemetry`
tests so the metric test runs before the trace test (avoids OTel
global-state pollution)
## Verification
- tests/server unit suite: 255 passed, 5 skipped
- tests/server/contract: 6 passed (baseline matches)
- tests/server/state redis suite: 29 passed (against docker redis)
- tests/server/integration:
- mock_mode_startup (file backend): 1 passed in 34s
- mock_mode_startup (redis backend): 1 passed in 34s
- mock_mode_startup (tinker SDK): 1 passed in 34s
- lgtm_telemetry: 18 passed in 12s (against grafana/otel-lgtm)
- full_sft_grafana_panels: 1 passed in 40s (against grafana/otel-lgtm)
* fix: use MultiLora state dict when saving adapters (#215)
* test(integration): full-cycle e2e — train + save + load + resume + vLLM sampler
Six-phase end-to-end harness against a real 4-app Ray Serve cluster
(server + transformer model + vLLM sampler + processor) on Qwen3.5-4B.
Walks every stateful surface of the client/server stack and asserts:
- Phase C reload-verify: load(ckpt_a) restores adapter weights so
fixed-batch loss matches A-fixed within RELOAD_LOSS_TOLERANCE. This
is the regression gate for #215 (the multi_lora save/load key-naming
fix) — before that fix the saved safetensors was empty (0 keys, 16
bytes) and Phase C would KeyError.
- Phase D resume-verify: resume_from_checkpoint(ckpt_b) restores
optimizer state + cur_step; two further training steps stay within
RESUME_LOSS_BAND of Phase B's last step.
- Phase E + F vLLM LoRA-effect greedy probe: for each PROBE_PROMPTS
entry, greedy-sample once with adapter_uri=None and once with
adapter_uri=ckpt_b. Assert at least one prompt diverges (token-level)
between base and adapter. Catches two silent failure modes the
earlier single-prompt + temp=0.7 check missed: (a) vLLM falls back to
base on LoRA load error; (b) training degenerated so ||B@A|| is too
small to move logits.
No pytest collection — needs real GPUs + externally-booted server, so
the file is named without ``test_`` prefix. Module docstring has the
full boot recipe (3-node Ray + 4-app serve via the new
server_e2e.py + server_config_e2e.yaml cookbook wrapper).
Co-located helpers:
- cookbook/client/server/transformer/server_config_e2e.yaml — 4-app
config (mirror of server_config.yaml with the vLLM sampler block
uncommented, dedicated persistence file_path so it doesn't clobber
the demo state).
- cookbook/client/server/transformer/server_e2e.py — boot wrapper
pointing launcher at the e2e yaml.
* fix: auto-inject model_id in sampler set_template (#216)
Sampler's set_template now auto-injects self.model_id when the caller
omits it, consistent with how the model backend already overrides
model_id with self.tokenizer_id. This eliminates the need for clients
to manually resolve the HF model ID when the route name differs from
the underlying model.
Also fixes two client-generator bugs: model client assigned self.model_id
before stripping the twinkle:// prefix, and sampler client was missing
self.model_id entirely.
Co-authored-by: xichengpro <188454548+xichengpro@users.noreply.github.com>
* feat: bundle Redis + LGTM + OTel into Docker image and server startup
- pyproject.toml: merge redis/telemetry extras into unified `server` group,
add opentelemetry-api/sdk/exporter-otlp/instrumentation-logging
- Dockerfile: multi-stage COPY redis-server from redis:7 and LGTM stack
from grafana/otel-lgtm; install twinkle with `.[server]` extras
- run.sh: replace standalone Prometheus with LGTM startup (Redis → Ray →
inject Ray scrape config → LGTM → Twinkle); guard against repeated
prometheus.yaml append via .orig backup
- server_config.yaml: enable telemetry (OTLP localhost:4317), switch
persistence to Redis on port 6380 (avoids Ray GCS 6379 conflict)
* update
* feat: OpenAI-compatible /v1/chat/completions endpoint with streaming
Gateway translates OpenAI chat format → SampleRequest and routes via
sticky session (model field = multiplex key). Streaming uses a new
/twinkle/sample_stream ndjson endpoint on the sampler, wrapped as SSE
by the gateway. 32 local tests pass (bridge + handler + contract).
* test: add OpenAI endpoint e2e script for remote A100 verification
* fix: auto-set sampler template on first OpenAI chat request
The vLLM sampler requires a chat template before encoding Trajectory
inputs. Gateway now lazily calls /twinkle/set_template on the sampler
(once per base_model) using get_template_for_model to infer the right
template class.
* fix: include adapter_name in set_template request body
The SetTemplateRequest Pydantic model requires adapter_name field
even though it defaults to empty string on the server's type definition.
* fix: use sampler.sample() remote call for streaming endpoint
The vLLMSampler runs as a Ray actor — cannot access .engine or
.template through the actor proxy. Use the existing sample() remote
function and emit the complete result as streaming ndjson chunks.
* fix: relax streaming assertion to accept single-chunk responses
Ray actor boundary prevents true token-by-token streaming —
sampler.sample() returns complete results. The streaming endpoint
works correctly but emits content in one chunk rather than many.
* test: add sticky session verification phase to openai e2e
Sends 5 rapid requests and asserts all hit the same warm replica
(no re-template-init delay), proving Serve-Multiplexed-Model-Id
header routes correctly.
* refactor(tests): remove redundant/over-mocked server tests
Removed 5 test files that either:
- Guard static import paths from a past refactor (now stable)
- Test trivial utilities already covered by higher-level tests
- Are purely mock-wiring assertions with no real behavior tested
Files removed:
- test_checkpoint_relocation_imports.py (import-path guards)
- test_launcher_package_imports.py (import-path guards)
- test_backend_dispatch.py (trivial utility, covered by mock_model/mock_sampler)
- test_lazy_cleanup_mixin.py (10-line flag, covered by integration test)
- test_telemetry_shutdown.py (mock call-count assertions only)
* feat: add X-Twinkle-Replica-Id header for sticky session verification
- Model and Sampler inject replica ID in response headers via middleware
- Gateway forwards the header in OpenAI endpoint responses
- E2E test Phase 5 now asserts all same-model requests hit the same
replica by checking X-Twinkle-Replica-Id consistency (replaces
timing-based inference)
* update
* refactor: fix isort config + upgrade pyupgrade to --py311-plus
- Remove incorrect known_standard_library=setuptools from isort config
- Fix known_third_party: remove json (it IS stdlib), keep yaml
- Upgrade pyupgrade from --py38-plus to --py311-plus
- Auto-fix import ordering and legacy typing across server code
* refactor: move PersistenceConfig and TelemetryConfig to config/ package
Break the config → state and config → telemetry import dependencies by
moving the pure Pydantic configuration models into the config/ package
where they belong alongside ServerConfig and ApplicationSpec.
* refactor: flatten checkpoint/base/ into checkpoint/
Move models.py, paths.py, training_run_manager.py, and
checkpoint_manager.py up one level, removing the unnecessary base/
sub-package indirection.
* refactor: move metrics_middleware from utils/ to telemetry/middleware
The metrics adapters and middleware are tightly coupled to
telemetry/metrics.py's MetricsRegistry — they belong in the
telemetry package, not in generic utils/.
* refactor: rename app_scaffold.py to deployment.py
"deployment" directly describes the module's purpose (building Ray Serve
deployments) rather than the generic "scaffold" naming.
* fix: code-level fixes from review
- Rename make_error() type param to error_type (shadows builtin)
- Prefix unused loop vars with _ in openai_bridge.py
- Remove hardcoded Qwen/Qwen3.6-27B default model from gateway
- Add logger.warning on timestamp parse failure in state/base.py
- Document non-atomic pop TOCTOU in config_manager.py
- Replace print() with logger.info() in server_launcher.py
- Add dict[str, str] type annotation in telemetry/provider.py
- Fix "avaliable" typo in common/router.py
- Change .. relative imports to absolute in model/app.py, sampler/app.py
- Remove unused Dict, Optional imports
- Add per-process scope comments on module-level mutable state
- Add autouse fixture to reset _template_initialized between tests
* docs: update server docs for config refactor + add observability docs
Sync existing documentation with PR #210 breaking changes:
- CLI: python -m twinkle.server --config → twinkle-server launch -c
- Config: use_megatron → backend field, add telemetry/persistence sections
- Add new Observability docs (zh + en) covering OTLP, Grafana, metrics
- Fix cookbook/observability/README.md field name (telemetry_config → telemetry)
* fix: remove roadmap references to pass no-roadmap-refs hook
Strip (R27), (R29.4), (R9.2) citations from test docstrings and
rename "Phase N:" to "Step N:" in openai_e2e.py print statements.
* Revert "fix: remove roadmap references to pass no-roadmap-refs hook"
This reverts commit c003d458551cc842dff420072419a0a7004078c6.
* revert: restore pyupgrade to --py38-plus
The --py311-plus upgrade rewrites ~180 files across the entire repo
which is out of scope for this documentation PR.
* fix: remove roadmap references to pass no-roadmap-refs hook
Strip (R27), (R29.4), (R1.1/R9.1/R9.2) citations from test docstrings
and rename "Phase N:" to "Step N:" in openai_e2e.py print statements.
* fix: address PR review comments
- docker-compose.yaml: telemetry_config → telemetry in comments
- task_queue/worker.py: catch asyncio.TimeoutError instead of broad TimeoutError
- sampler/twinkle_handlers.py: MODEL_ID correlation uses self.model_id
instead of adapter name
- cookbook configs: TWINKLE_TRUST_REMOTE_CODE default to "0" for safety
* feat: implement true token-level streaming for /twinkle/sample_stream
Previously the endpoint waited for full completion via sampler.sample()
then emitted results as fake streaming chunks. Now it consumes
VLLMEngine.generate_stream() token-by-token via a cross-event-loop
queue bridge in vLLMSampler.sample_stream().
- vLLMSampler: add sample_stream() that bridges the background event
loop to callers using queue.Queue
- twinkle_handlers: rewrite /twinkle/sample_stream to consume the
real async generator
- MockSampler: add sample_stream() for duck-type compatibility
- Tests: 3 new tests for mock streaming interface
* refactor: extract _iter_in_loop helper + fix sample_stream for server mode
- vLLMSampler: extract _iter_in_loop() as the generator counterpart to
_run_in_loop(), simplifying sample_stream() to a yield-from one-liner.
Fix template access with getattr for safety.
- twinkle_handlers: sample_stream falls back to sample() in server mode
because Ray's @remote_class proxy cannot forward Python generators
across process boundaries. True token-level streaming requires
DeploymentHandle.options(stream=True) which is a future architecture
change.
* feat: mock backends use @remote_class + queue-based streaming
- Add @remote_class() to MockSampler and MockModel so they run as
Ray Actors (CPU DeviceGroup), matching the real backends' process
boundary behavior.
- SamplerManagement/ModelManagement init paths now always call
twinkle.initialize() for both mock and non-mock backends.
- Rewrite /twinkle/sample_stream to use ray.util.queue.Queue for
cross-process token streaming (works for both mock and vLLM).
- Add sample_stream_to_queue() to both vLLMSampler and MockSampler.
- Remove obsolete import-isolation tests and stale roadmap refs.
- Clean up outdated docstrings (numpy-only, avoid-torch, etc.).
* fix: handle sync get_tokenizer() in vLLM 0.19.0
vLLM 0.19.0 returns a sync CachedTokenizersBackend from
engine.get_tokenizer() instead of a coroutine. Check for
__await__ before awaiting.
* chore: remove no-roadmap-refs pre-commit hook
* fix: template None guard in _sample_single + case-insensitive device_type check
- vllm_sampler._sample_single: fall back to raw input_ids when
self.template is None (consistent with sample_stream behavior).
- ResourceManager: use .upper() for device_type comparison in the
device_groups loop, matching the earlier ranks loop.
* refactor: extract STREAM_SENTINEL to shared constant
* fix: ensure contiguous tensors before safetensors save
safetensors.save_file() requires contiguous tensors but multi-lora
adapter state dicts can contain non-contiguous views, causing ValueError
on save. Call .contiguous() on each tensor before serialization.
* chore: set TWINKLE_TRUST_REMOTE_CODE=1 in e2e config
The safe mode check_unsafe() rejects internal worker calls that pass
model objects (Callable) to the processor, breaking forward_backward.
Set trust=1 to unblock GRPO testing while the infra-level fix is TBD.
* refactor: extract shared header constants, drop redundant X-Ray-Serve-Request-Id
- New twinkle_client/http/headers.py centralises header name constants
and build_routing_headers() used by 3 call sites
- Remove X-Ray-Serve-Request-Id — redundant since Ray Serve 2.55+ reads
x-request-id; updated validation.py to match
- Fixes _build_sticky_headers missing x-request-id / serve_multiplexed_model_id
* feat: pass adapter_uri through OpenAI chat/completions endpoint
Allows users to specify a twinkle:// checkpoint path for LoRA inference
via the OpenAI-compatible endpoint. When adapter_uri is omitted, the
sampler uses the base model as before.
* fix: use token/session_id as sticky_key for OpenAI sampler routing
Using the model name as sticky_key caused all callers to share the same
adapter slot and produced nonsensical full_adapter_names. Switch to
session_id (preferred) or token so each client is isolated, matching
what twinkle_client d…
Collaborator
|
The core changes from this PR — sampler Specifically:
Closing this PR. Thanks @xichengpro for the original fix. |
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
2 participants
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.
PR type
PR information
Write the detail information belongs to this PR.
template_init_model_idtoSupportedModelto support configuring the target model ID used for template initializationtemplate_model_idhelper module, preferring an explicit model ID and otherwise resolving it from server capabilitiesMultiLoraTransformersModel.set_templateandvLLMSampler.set_templateto replace model ID passing with the new resolution flowExperiment results
Paste your experiment result here(if needed).