-
Notifications
You must be signed in to change notification settings - Fork 276
feat(opensandbox): keepalive-bounded transport + create hardening + resource requests/limits #2212
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
Changes from all commits
4d59514
fd0e74e
1d3c7c6
b09424e
2f5cc32
a515050
e4c5f9d
4822006
3136024
cb477c5
aa2879e
3c5eb72
3eb2171
3e8e5e6
15d6464
ec3e827
446bec8
63fe66f
f413ae8
d27f5fc
cf1bcbb
da6b17b
e8ef37d
d663404
3698bcf
83464fd
e69b029
717de2a
f3cdeb4
05c1cae
eebf8fa
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 |
|---|---|---|
|
|
@@ -12,11 +12,11 @@ | |
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| import asyncio | ||
| import hashlib | ||
| import json | ||
| import os | ||
| import sys | ||
| import threading | ||
| import time | ||
| import traceback | ||
| from asyncio import Semaphore | ||
|
|
@@ -89,6 +89,10 @@ class MiniSWEAgentVerifyResponse(BaseVerifyResponse): | |
|
|
||
|
|
||
| @ray.remote( | ||
| # Rollout tasks spend nearly all their time waiting on LLM calls and | ||
| # sandbox I/O; reserving a full CPU per task caps concurrent rollouts at | ||
| # the Ray cluster's core count long before any real resource limit. | ||
| num_cpus=0.25, | ||
| scheduling_strategy="SPREAD", | ||
| runtime_env={ | ||
| "py_executable": sys.executable, | ||
|
|
@@ -509,6 +513,11 @@ def _run_mini_swe_v2(**params: Any) -> dict[str, Any]: | |
| model_kwargs = model_config.setdefault("model_kwargs", {}) | ||
| model_kwargs["api_key"] = params["api_key"] | ||
| model_kwargs["base_url"] = params["base_url"] | ||
| # Bounded retries for transient LLM-call failures (disconnects, resets): | ||
| # without any retry a single failed call kills the whole rollout, while a | ||
| # large value makes litellm retry silently for so long that the rollout | ||
| # looks hung. Config-provided model_kwargs take precedence. | ||
| model_kwargs.setdefault("num_retries", 5) | ||
| model_kwargs.pop("api_base", None) | ||
| max_output_tokens = model_kwargs.pop("max_output_tokens", None) | ||
| if max_output_tokens is not None and "max_tokens" not in model_kwargs: | ||
|
|
@@ -578,7 +587,18 @@ def _run_mini_swe_v2(**params: Any) -> dict[str, Any]: | |
| } | ||
| finally: | ||
| if env and hasattr(env, "cleanup"): | ||
| env.cleanup() | ||
| # Off the critical path: this finally block runs before the task's | ||
| # return value becomes fetchable, so an in-band stop() delays every | ||
| # finished result and, on failure, re-raises over it. Orphans are | ||
| # covered by the provider's sandbox TTL. | ||
| threading.Thread(target=_cleanup_env_best_effort, args=(env,), daemon=True).start() | ||
|
|
||
|
|
||
| def _cleanup_env_best_effort(env: Any) -> None: | ||
|
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. NOTE — sandbox teardown moved to a fire-and-forget daemon thread. Motivation is sound (a slow/hanging commit_id: 3eb2171
Contributor
Author
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. Deliberate tradeoff, keeping it as-is for this PR — but the analysis is fair and worth recording. Why the daemon thread: the in-band On the leak window, you are right that it is not zero and not identical to the old behavior. With in-band cleanup, teardown finished before the task returned, so worker teardown could not interrupt it. Fire-and-forget genuinely widens the window: on autoscale-down, eviction, or worker crash the thread can die mid- On TTL sizing: agreed that 5h is provisioned for long rollouts rather than for churn, and that it is the right knob if orphan pressure shows up. Leaving it unchanged here so this PR stays scoped to the transport and delivery path; sizing it against observed churn is a config-level follow-up for the author to make with real numbers rather than a guess. |
||
| try: | ||
| env.cleanup() | ||
| except Exception as e: | ||
| print(f"[CLEANUP] best-effort sandbox teardown failed: {e}", flush=True) | ||
|
|
||
|
|
||
| def run_mini_swe_with_sandbox(**params: Any) -> Any: | ||
|
|
@@ -807,7 +827,12 @@ async def run(self, body: MiniSWEAgentRunRequest) -> MiniSWEAgentVerifyResponse: | |
| if runtime_env.get("env_vars"): | ||
| runner = runner.options(runtime_env=runtime_env) | ||
| future = runner.remote(run_mini_swe_with_sandbox, params) | ||
| result = await asyncio.to_thread(ray.get, future) | ||
| # Ray ObjectRefs are awaitable: park on the event loop instead | ||
| # of pinning a thread in asyncio's default executor (capped at | ||
| # min(32, cpu+4) workers). With the thread-blocking ray.get, | ||
| # at most ~32 rollouts can be waiting on results at once and | ||
| # every other finished task queues behind them. | ||
| result = await future | ||
| result = result[instance_id] | ||
| input_messages = result["input_messages"] | ||
| response_output = result["response_output"] | ||
|
|
||
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.
NOTE — fire-and-forget teardown can leak sandboxes at scale.
WHAT:
env.cleanup()now runs in a detached daemon thread instead of synchronously infinally. The Ray worker is free to return (and be reused or idle-killed) before the thread finishes.BLAST RADIUS: For the common case the thread completes fine (workers are long-lived). But any cleanup that doesn't finish before its worker dies leaks a sandbox until TTL — and
sandbox_spec.ttl_sis 18000s (5h). On a large eval, a burst of dropped teardowns near run-end could pin quota for hours. The_cleanup_env_best_effortswallow also means a persistently-failing teardown is invisible except in stdout.FIX: Acceptable as a deliberate tradeoff given the TTL backstop — the comment already acknowledges it. If quota pressure shows up, consider a bounded join at run shutdown or draining outstanding cleanup threads. No change required to merge.
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.
Same teardown tradeoff as the earlier thread on this block (the line moved 603 -> 600 with the bypass refactor) — answered there: #2212 (comment)
Short version: the daemon thread is deliberate because the in-band
stop()infinallyran before the task result became fetchable, so it gated every finished rollout and destroyed successful results when it raised. The wider orphan window is accepted and backstopped by the provider TTL plus an out-of-band sweep; TTL sizing is a config-level follow-up to be made against observed churn. Agreed it is not a merge blocker.