-
Notifications
You must be signed in to change notification settings - Fork 420
[tinker] Encode forwarded sample results to proto once and serve them as-is #2164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: avi/stack-4-server-knobs
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,7 +16,11 @@ | |||||||||
| from skyrl.tinker import types | ||||||||||
| from skyrl.tinker.config import EngineConfig | ||||||||||
| from skyrl.tinker.db_models import EngineStateDB, FutureDB, RequestStatus | ||||||||||
| from skyrl.tinker.external_future_store import ExternalFutureStore | ||||||||||
| from skyrl.tinker.external_future_store import ExternalFutureStore, PreparedResult | ||||||||||
| from skyrl.tinker.proto_serialization import ( | ||||||||||
| sample_output_json_from_proto, | ||||||||||
| serialize_sample_output, | ||||||||||
| ) | ||||||||||
| from skyrl.utils.log import logger | ||||||||||
|
|
||||||||||
|
|
||||||||||
|
|
@@ -142,12 +146,15 @@ async def call_and_store_result( | |||||||||
| logger.warning("FutureDB row %s missing on completion write — skipping", request_id) | ||||||||||
| return | ||||||||||
| # `result_data` is a text column holding pre-serialized JSON. | ||||||||||
| future.result_data = result.model_dump_json() | ||||||||||
| if isinstance(result, PreparedResult): | ||||||||||
| future.result_data = result.json or sample_output_json_from_proto(result.proto) | ||||||||||
|
Comment on lines
+149
to
+150
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If
Suggested change
|
||||||||||
| else: | ||||||||||
| future.result_data = result.model_dump_json() | ||||||||||
| future.status = status | ||||||||||
| future.completed_at = datetime.now(timezone.utc) | ||||||||||
| await session.commit() | ||||||||||
|
|
||||||||||
| async def _forward_with_retry(self, sample_req, model_id: str, *, base_model: str | None) -> types.SampleOutput: | ||||||||||
| async def _forward_with_retry(self, sample_req, model_id: str, *, base_model: str | None) -> PreparedResult: | ||||||||||
| # Retry only failures where the request demonstrably did not execute: | ||||||||||
| # connect-phase errors and 5xx rejections from the router. Read and | ||||||||||
| # write failures are ambiguous: vLLM may still be executing the | ||||||||||
|
|
@@ -179,9 +186,7 @@ async def _forward_with_retry(self, sample_req, model_id: str, *, base_model: st | |||||||||
| "the SKYRL_FORWARDING_INFERENCE_TIMEOUT_SEC environment variable." | ||||||||||
| ) from e | ||||||||||
|
|
||||||||||
| async def _forward( | ||||||||||
| self, proxy_url: str, sample_req, model_id: str, *, base_model: str | None | ||||||||||
| ) -> types.SampleOutput: | ||||||||||
| async def _forward(self, proxy_url: str, sample_req, model_id: str, *, base_model: str | None) -> PreparedResult: | ||||||||||
| # model_id matches the LoRA name registered with vLLM during | ||||||||||
| # save_weights_for_sampler; base_model is used for non-LoRA sampling. | ||||||||||
| model_name = base_model if base_model else model_id | ||||||||||
|
|
@@ -266,16 +271,8 @@ async def _forward( | |||||||||
| # Tinker's stop_reason is Literal["stop", "length"]; vLLM emits a wider set. | ||||||||||
| finish_reason = choice.get("finish_reason") | ||||||||||
| stop_reason = "stop" if finish_reason in ("stop", "stop_token") else "length" | ||||||||||
| sequences.append( | ||||||||||
| types.GeneratedSequence( | ||||||||||
| tokens=tokens, | ||||||||||
| logprobs=logprobs, | ||||||||||
| stop_reason=stop_reason, | ||||||||||
| ) | ||||||||||
| ) | ||||||||||
| sequences.append((stop_reason, tokens, logprobs)) | ||||||||||
|
|
||||||||||
| return types.SampleOutput( | ||||||||||
| sequences=sequences, | ||||||||||
| prompt_logprobs=prompt_logprobs, | ||||||||||
| topk_prompt_logprobs=topk, | ||||||||||
| ) | ||||||||||
| # Encode straight to the proto wire form the SDK retrieves; no pydantic | ||||||||||
| # model or JSON text is built for the result (see PreparedResult). | ||||||||||
| return PreparedResult(proto=serialize_sample_output(sequences, prompt_logprobs, topk)) | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If retrievals for the same JSON-backed result overlap, each waiter can observe the empty proto cache before acquiring
proto_serialization_lock, causing the same large result to be serialized sequentially multiple times and adding avoidable CPU and retrieval latency.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!