Conversation
…es work T0.1: capture current HEAD client-facing surface so the post-refactor comparison (T8.1) shows only this spec's changes. No code changes.
…1.6) - execute_all_sync forwards timeout to ray.get (T1.1) - resolve effective ray.get timeout before choosing execute_method and flip priority to 'decorator wins, instance is fallback' at both dispatch sites; fix the 0-treated-as-falsy trap; bound __len__/__next__ bare ray.get (T1.2, T1.5) - decorator timeout=10 on ping; timeout=3600 on save/add_adapter_to_model/ resume_from_checkpoint/tinker_load/load_full_weights_from_path (T1.3) - TaskQueueConfig default execution_timeout 120->1800; effective_execution_timeout (0 -> 3600) as the single bound source; startup warning on 0 (T1.4) - infra unit tests, no GPU/Megatron/server deps (T1.6)
…2.1-T2.4, T6.1) - twinkle_client/types/base.py: StrictRequest/ResponseModel/DataModel + backend_only() helper/reader; naming rulings in module docstring (defined, not applied) (T6.1) - twinkle_client/types/errors.py: ErrorCategory + ErrorPayload(ResponseModel) (T2.1) - task_errors.py: task_error_payload builds ErrorPayload dict (request_id/error_code, traceback split+tail-trim, User carries no traceback); error_payload_from_stored backfills legacy two-field payloads without ValidationError (T2.2) - worker: single-line error summary + full traceback in traceback field; TimeoutError and Ray_Get_Timeout -> 504/Server, others -> 500/Server (T2.3) - tests for ErrorPayload + updated task_errors test (T2.4)
…e 6, T6.2-T6.3) - QueueStateLiteral in types/errors.py, values sourced to match server QueueState; consistency test asserts equal value sets (T6.2) - T6.3 realized as a guard test (per user ruling): twinkle_client already shares 18 public names with tinker.types by design (tinker-compatible client), so the literal 'no intersection' cannot hold without renaming twinkle. Guard instead asserts no src/twinkle module binds a tinker and a twinkle_client type to the same local name (tinker must be aliased when both coexist).
…T3.8) - call_backend: dedicated ThreadPoolExecutor (no max_workers=1) + per-replica opt-in Admission_Gate; gate released from the worker thread's finally so a wait_for-cancelled coroutine cannot free it while the call is still in flight; fast-fail BackendBusyError when the gate is held by a leaked call (T3.1) - ModelManagement enables the gate; SamplerManagement does not - worker maps BackendBusyError -> 503/Server - mechanical: 27 model/twinkle + 14 model/tinker + 8 sampler/twinkle + 4 sampler/tinker + 3 model/app direct backend calls -> await call_backend (T3.2-T3.5) - check_model_health async + admit=False ping; _cleanup_adapter via gate; /healthz awaits (T3.5, T3.6) - AST static check over src/twinkle/server/** + shared exemptions file (T3.7) - blocking-boundary integration tests (T3.8)
… T4.1-T4.3) - set _ray_get_timeout = effective execution timeout on model/sampler backends, effective for both sync and async dispatch (T4.1) - ComputeWorker fires an optional on_backend_timeout hook after a timeout; ModelManagement probes actor liveness (admit=False ping) and sets a health bit that /healthz reflects (503) and a successful probe auto-clears (T4.2) - worker skips a dequeued task whose record is already terminal (R3#8); document record-terminal (queue_timeout+T) vs resource-release (Collect_Width*T) bounds in EN+ZH Server docs (T4.3)
…1-T5.6) - FutureRecord.replica_id set at creation, never overwritten (T5.1) - ReplicaRegistry last_seen as a separate key (max_loras type unchanged); refreshed in _on_request_start; ModelManager.get_alive_replica_ids (T5.2) - cleanup_expired rewrite: never deletes a non-terminal record; orphans and over-absolute-ttl records are written failed; signature adds alive_replica_ids + absolute_ttl; ServerState.set_execution_bounds injects queue_timeout/T/ Collect_Width; age uses the stored-timestamp clock convention (T5.3) - do-not-regress guard extended to terminal->terminal (warn on different, silent drop on same) (T5.4) - sampler _stream_generator: 60s per-get + total-lifetime bound, ray Queue shutdown in finally, empty-actor structured error (T5.5) - state hygiene tests via Ray-free FileBackend (T5.6)
- delete TWINKLE_FAIL_FAST from the 4 cookbook server configs (T7.1) - delete Layer 1 (safe_loss/SafeLossWrapper/_zero_loss) + OptimizerGroup.__setattr__ auto-wrap hook + the test_micro_batch safe_loss import/case (T7.2) - delete Layer 2 (@nccl_safe decorator, _force_zero_backward, _iter_model_params) and its two decoration points + import in transformers_model (T7.3) - delete Layer 4: the forward_step_func post-processing try/except in megatron.py, exceptions now propagate; drop the _is_fail_fast import (T7.4) - delete Fail_Fast_Switch: _is_fail_fast + env_propagation NCCL_SAFE_ENV_KEYS/ build_nccl_safe_env_vars and its call (T7.5) - nccl_safe_megatron rewritten to unconditional rank-attributed log + re-raise, no tinker/forward_only params, no degraded return; module docstring records the lost coverage window (T7.6, T7.9) - rewrite both GPU-gated e2e tests to assert failed-terminal + subsequent-success and drop degradation symbols (T7.7) - unified static check asserting the 8 symbols are absent (T7.8)
…e 8, T8.1/T8.2/T8.4) - test_client_api_contract.py: OpenAPI surface == pre-impl baseline (T0.1), schedule_task_and_wait retained, only new client modules base.py/errors.py (T8.1) - new checks are auto-run by CI 'pytest tests'; lint.yaml runs pre-commit --all-files; applied yapf/isort/pyupgrade auto-fixes so all hooks pass on changed src files (T8.2) - tests/server/README.md documents the mock-backend evidence boundary (T8.4)
…endent Root cause: _now_iso() wrote naive local time while _parse_timestamp() reads a naive ISO string as UTC; compared against a time.time()-based cutoff this skewed every expiry check by the host's UTC offset (premature deletion west of UTC, over-retention east; invisible on UTC/CI hosts). Fix (option A): _now_iso() now emits UTC-aware ISO; future_manager reuses it for record writes and its cleanup 'now' reverts to time.time() (both UTC epoch). _parse_timestamp is unchanged and still parses legacy naive records as UTC (same as before). Not a wire-schema change (R8 unaffected). Adds a timezone-independent regression test asserting a freshly written record parses to within 1s of time.time(); the suite passes under TZ=America/Los_Angeles.
# Conflicts: # src/twinkle/server/model/app.py
…m/gen bounds) Complete the server-execution-guarantees spec by routing the remaining sampler paths through the Blocking_Call_Boundary and bounding every long-lived operation: - sampler: submit/collect/cancel generation, unload, and streaming now go through call_backend; _await_generation is bounded by the effective execution timeout and _stream_queue enforces a double timeout (total + per-token) instead of blocking indefinitely. - task queue: thread collect_width through _init_task_queue so state hygiene computes the absolute survival TTL without a separate set_execution_bounds hop. - errors: stream/generation failures surface as ErrorPayload wire frames rather than silent drops. - deps: bump tinker to 0.29.0 (python>=3.11) and refresh poetry.lock. - tests/docs: add error-wire, stream-guarantee and nccl_safe coverage; refresh the client API contract baseline (zero wire change).
tinker 0.29.0 turned its tensor request types into dataclasses, which breaks FastAPI OpenAPI generation. Revert the pin and the code that had been migrated to the 0.29.0 API: - pyproject: tinker 0.29.0 -> 0.16.1; restore poetry.lock to match - sampler/tinker_handlers: use 0.16.1 SampledSequence/SampleResponse fields - contract harness: fastapi 0.136 ModelField compat; normalize :path route converters so the baseline stores client-facing paths - regenerate client_api_baseline.json against tinker 0.16.1 - test fixtures: add call_backend/_task_queue_config; drop 0.29-only assertion
…ecycle Implements the server-request-lifecycle spec (Part 2). A single HTTP request's server-side duration is now decoupled from task execution time: submit enqueues and returns a TaskEnvelope immediately, and the client's future layer polls a dedicated retrieve endpoint. The motivation is narrow and load-bearing: the client's per-request timeout was 600s while most ingress gateways cut idle connections at 60s, so data-plane endpoints (forward_backward, sample) were already unusable behind a real gateway. Every single HTTP request is now bounded by the 30s long-poll window regardless of how long the task runs. This does not improve throughput or training speed -- the compute queue is still serial and GPU utilisation is unchanged. Server: - new twinkle/server/lifecycle/: envelope.py (the one FutureRecord -> TaskEnvelope mapping point), poll_config.py (single declaration of the long-poll window and interval, shared by both retrieve endpoints), submit.py (run_submit shell plus the to_backend_inputs / backend_kwargs / input_metrics seams left for Part 3) - new POST /twinkle/retrieve_future and POST /twinkle/cancel - preflight now raises RequestRejectedError subclasses, so a rejected request returns a real status code and writes zero future records - TwinkleServerError handler puts ErrorPayload fields at the response top level - delete schedule_task_and_wait, run_task (both copies), QueuedTask.completion, _complete_result/_complete_error and persist_status; the future record is now the only delivery channel for results and failures - delete TaskStatus.RATE_LIMITED (limiting is now HTTP 429), the get_state_dict endpoint and the upload_status endpoint Client: - new types/lifecycle.py (TaskEnvelope), _future.py (the only polling loop), exceptions.py (TwinkleHTTPError / TaskFailedError / TaskCancelledError / TaskWaitTimeoutError / TaskRecordLostError) - three separate 600s timeout literals collapse into _HTTP_TIMEOUT = 90 - public methods keep their synchronous signatures and return types, so cookbook scripts and integration tests are unchanged Breaking changes: queued endpoints return TaskEnvelope instead of a business model; task failure raises TaskFailedError (HTTP 200 + payload) instead of requests.HTTPError (HTTP 500); get_state_dict is removed (use save + read the checkpoint). Verified on real PPU hardware (Qwen3.5-4B, 8x ZW810): SFT/DPO/GRPO x twinkle/tinker on the transformers backend and SFT x twinkle/tinker on megatron, 8/8 passing with losses identical to the pre-refactor run. Unit suite: 360 passed, 0 failed.
…ixins - move sampler weight resolution to sampler/weights.py and streaming bridge to sampler/backends/streaming.py so handlers stay thin - rename utils/lifecycle to utils/session_resource: the package holds session-scoped resource mixins, not the request lifecycle owned by twinkle/server/lifecycle - add static guards for adapter-name mapping and package-root imports - declare grimp test dependency used by the import-boundary guards
…ing disambiguation Move HTTP-boundary-decidable request problems out of the async training path: - Single shared request models (twinkle_client.types) with field roles (control / backend_kwarg / passthrough); client builds via build_request + model_dump_json instead of hand-assembling json_data. - StrictRequest on all twinkle-native routes; unified RequestValidationError -> ErrorPayload (422/501) registered on the shared deployment app builder. - Wire schema for inline `inputs` validated at HTTP ingress (strict int leaves, homogeneous batch, extension-field preservation); single shared `is_encoded` predicate in twinkle.data_format.encoding replaces three copies. - run_submit preflight (assert_request_supported) rejects backend-incompatible fields and unavailable endpoints before seq claim / enqueue -> zero DP ranks. - Passthrough keys forwarded unjudged (no spelling heuristic). Naming disambiguation (client/server contract fix): - Prefix sampler-domain models (SamplerAddAdapterRequest / SetTemplate* / CreateResponse) so they no longer collide with model.py; the sampler handler now binds sampler_types explicitly. Fixes add_adapter_to_sampler validating against model.py's `config: Optional[str]` and rejecting the dict the client sends. - Remove dead model.AddAdapterResponse and server.WeightsInfoResponse. - training.py response envelopes inherit ResponseModel. - Regenerate contract route inventory; add regression tests pinning the sampler binding and the dict-config acceptance.
Auto-fixes flagged by CI on b928a17 (pre-commit run --all-files): - sampler/twinkle_handlers.py: isort import order + wrap the 122-char create() signature (E501). - validation/backend_compat.py: pyupgrade Optional[str] -> str | None.
…ing code Remove src/twinkle_client from the pre-commit exemptions (flake8/isort/yapf/ pyupgrade + the whitespace/EOL/quote fixers) so the client is linted like the server, and bring the existing (previously-unchecked) client code to a green `pre-commit run --all-files`: - isort/yapf/pyupgrade/double-quote normalization across the client package. - Wrap over-length tool/description/error strings (implicit concatenation, content preserved) in auto/agent/tools.py and utils/patch_tinker.py. - Rename ambiguous loop var `l` -> `line` (E741) in auto/agent/monitor.py. - `# noqa: E402` on the intentional late import in twinkle_client/__init__.py. - setup.cfg per-file-ignores: E501 for auto/agent/monitor.py (embedded LLM prompt with verbatim long lines). - types/__init__.py: scoped `# yapf: disable` around the re-export block so the 45-name `from .model import (...)` stops oscillating between isort's aligned wrap and yapf's hanging wrap (isort still owns the ordering).
…ut cleanup Server: - Rename utils/validation.py -> utils/auth.py to end the collision with the server/validation preflight package (auth/session helpers vs request checks). - Rename model/utils.py -> model/data_plane_inputs.py to name what it does. Client: - Rename http/http_utils.py -> http/client.py and http/utils.py -> http/context.py; update all importers. - Split auto/agent/tools.py into tool_schemas.py (schemas), server_tools.py and search_tools.py (ToolExecutor mixins); add test_auto_agent_tools.py. - Rework rollout/multi_turn.py into an explicit _RolloutState with _initialize_state / _process_sequence helpers. All touched files pass `pre-commit run --all-files` (client now linted).
Yunnglin
changed the title
…pe; repair PPU full test suite - multi_lora: mirror PeftModel.__init__ adapter dtype autocast on each add_adapter slot (drop unconditional float() normalization) - transformers: make _ensure_lora_dtype a @staticmethod; update call site - align tests with PEFT 0.18.1 target-parameter shapes and transport API; loosen slow-startup/backend timeouts - refresh twinkle client cookbooks/docs for client-as-factory usage
…ng R6-R13) - R6: bounded fail-open session liveness via last_liveness_confirmed_at - R7: full ABC hooks + cluster-global processor lease quota (429/User) - R8: ConcurrencyError->StateBackendError(503); close() releases handle only, flush_all() for teardown; backend contract docstrings - R9: drop **kwargs pseudo-polymorphism; ModelManager quota -> 429/User - R10: Twinkle-native ErrorPayload single exit; EndpointUnavailableError moved to server/exceptions.py; no-HTTPException static guard - R11: FutureFailureRecord domain failure; protocol-boundary wire mapping - R12: client ErrorPayload parse with details/traceback, lowercase category - R13: remove unrunnable sampler 'torch' option Verified: unit/contract regression (361 passed) + full 2x2x3 E2E matrix (transformers+megatron x twinkle+tinker x sft/dpo/grpo, 12/12 passed incl. save-LoRA/state + resume).
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
1 participant
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.
Summary
Hardens the Twinkle server's entire request-serving path across three sequenced specs, then enforces lint on the client and lands the supporting refactors. Client and server share one request-model contract; every serving path has a strict time bound; failures propagate loudly.
ErrorPayloadinstead of degrading silently; a shared pydantic contract base.TaskEnvelope; results are delivered through a persistent future record + retrieve endpoint, decoupled from the HTTP connection.Key changes
1. Execution guarantees (time bounds + loud failures)
sync=Truedispatch path; blocking calls moved off the event loop viacall_backend+ a per-replica admission gate; AST guard forbids new direct backend calls.TWINKLE_FAIL_FASTswitch;nccl_safereduced to a singlenccl_safe_megatrondecorator.ErrorPayloadwire frames.2. Async submit/retrieve lifecycle (Part 2)
run_submit/submit_and_peeksubmit shell: handlers submit and return immediately; the client future layer resolves the envelope to a terminal state.FutureRecord(withreplica_id/absolute_deadline); cleanup reads each record's own deadline; do-not-regress guard extended to terminal->terminal.seq_iddedup keyed on(session, adapter, seq_id)so a retried gradient-mutating call applies at most once.3. Request schema (Part 3)
twinkle_client.types; the client builds them viabuild_request+ onemodel_dump_json(exclude_none=True)instead of hand-assemblingjson_data.StrictRequeston all twinkle-native routes (unknown top-level field -> 422);ResponseModel(extra='ignore') so older clients tolerate new response fields;tokenis rejected in the body.backend_kwarg/ passthrough): each backend field forwarded at most once, control fields never forwarded, passthrough regions flattened and forwarded unjudged (no spelling heuristic).run_submitpreflight rejects backend-incompatible fields (422) and unavailable endpoints (501) before the seq claim / enqueue -> a rejected request runs on zero data-parallel ranks.inputsat HTTP ingress: strict integer leaves (reject bool/float), homogeneous batch, unknown-JSON-field preservation on export, shallowest-first unions; a single sharedis_encodedpredicate replaces three copies.RequestValidationError -> ErrorPayloadhandler registered on the shared deployment app builder (Model / Sampler / Processor identical).Sampler-prefixed so they no longer collide withmodel.py; fixesadd_adapter_to_samplersilently binding the wrong (model) schema and rejecting a dictconfig.4. Lint enforcement & refactors
src/twinkle_client/from the pre-commit exemptions; the client is now linted like the server, and the existing client code is normalized to green.server/utils/validation.py -> auth.py(ends the collision with theserver/validationpreflight package) andmodel/utils.py -> data_plane_inputs.py.http/http_utils.py -> http/client.pyandhttp/utils.py -> http/context.py; update all importers.auto/agent/tools.pyintotool_schemas.py/server_tools.py/search_tools.py; reworkrollout/multi_turn.pyinto an explicit_RolloutState.Breaking changes
Testing
tokenrejection,seq_idacceptance, sampler-binding regression), wire-schema (classification, homogeneity, strict ints, extension round-trip,CORE_INPUT_KEYS/VLM_CONCAT_FIELDSconsistency).nccl_safe, blocking-boundary + actor-recovery integration, future-lifecycle / error-payload state tests, and the zero-wire-change contract baseline guard.pre-commit run --all-filesgreen across server + client; local unit suites green in the vLLM env.