Skip to content

fix(tinker): align forwarding pool deadline - #2140

Open
hershg wants to merge 1 commit into
NovaSky-AI:mainfrom
hershg:hersh/tinker-forwarding-pool-timeout
Open

fix(tinker): align forwarding pool deadline#2140
hershg wants to merge 1 commit into
NovaSky-AI:mainfrom
hershg:hersh/tinker-forwarding-pool-timeout

Conversation

@hershg

@hershg hershg commented Sep 2, 2026

Copy link
Copy Markdown
Contributor
  • Use the configured inference deadline while a forwarded request waits for an HTTP connection, not only after dispatch.
  • Prevent bounded high-concurrency forwarding from failing at an unrelated hard-coded 300-second pool deadline.

Testing

uv run --isolated --frozen --extra dev --extra tinker --extra ray --extra jax pytest -q tests/tinker/test_inference_forwarding_config.py

The updated timeout-wiring assertion and local static checks pass; upstream CI provides the clean-environment suite.


Note

Low Risk
Narrow httpx timeout wiring change for inference forwarding; no auth or data-path logic, only avoids failing earlier than the configured inference deadline when the connection pool is saturated.

Overview
Fixes premature pool timeouts when forwarded inference requests wait for a free HTTP connection under high concurrency.

The forwarding client’s httpx.Timeout pool deadline was hard-coded to 300s while read already used forwarding_inference_timeout_sec. Pool is now set to the same configured value, so raising --forwarding-inference-timeout-sec / SKYRL_FORWARDING_INFERENCE_TIMEOUT_SEC applies while waiting for a connection as well as while reading the vLLM response.

Tests were updated to assert read and pool both match the configured timeout (e.g. 1800s).

Reviewed by Cursor Bugbot for commit e7c9f42. Bugbot is set up for automated code reviews on this repo. Configure here.

Signed-off-by: Hersh Godse <hersh@trajectory.ai>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the httpx client configuration in skyrl_train_inference_forwarding.py to use the configured forwarding_inference_timeout_sec for the pool timeout instead of a hardcoded value of 300.0. The corresponding unit tests in test_inference_forwarding_config.py have been updated and renamed to reflect this change. There are no review comments, and I have no additional feedback to provide.

@hershg

hershg commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

CI follow-up: code quality, gym, train, and the full SkyRL CPU suite pass on the final commit. The GPU workflow failed before submitting any remote job because the fork context has no Anyscale credentials; it did not reach GPU setup or tests. The candidate module was also loaded against the exact deployed runtime and confirmed connect/read/write/pool timeouts of 10/1800/300/1800 seconds. The unchanged 512-request B300 replay is in progress.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant