[Harbor] Add rate limit for trials/sec and max concurrency#1074
[Harbor] Add rate limit for trials/sec and max concurrency#1074CharlieFRuan merged 5 commits intoNovaSky-AI:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a robust rate and concurrency limiter for trajectory submissions, which is a valuable addition for managing environment constraints. The implementation uses a token bucket algorithm for rate limiting and an asyncio.Semaphore for concurrency control. I have provided feedback on the manual lock management in the AsyncRateLimiter class, which can be simplified to improve maintainability and robustness against cancellation issues.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| # Initialize rate limiter | ||
| rate_limit_config = terminal_bench_cfg.get("rate_limit", None) | ||
| self._rate_limiter = create_rate_limiter(rate_limit_config) |
There was a problem hiding this comment.
🔴 SkyRL-specific rate_limit config leaks into Harbor's TrialConfig.model_validate(), potentially causing validation errors
When a user enables rate limiting by adding a rate_limit section to their terminal bench YAML config, this SkyRL-specific field is included in _harbor_config_template (created at terminal_bench_generator.py:63 via OmegaConf.to_container(terminal_bench_cfg, resolve=True)) and then passed to Harbor's TrialConfig.model_validate(config) at terminal_bench_generator.py:169.
Root Cause and Impact
At line 63, the entire terminal_bench_cfg is converted to a plain dict:
self._harbor_config_template = OmegaConf.to_container(terminal_bench_cfg, resolve=True)At lines 92-94, rate_limit is read from the same config but NOT removed from the template:
rate_limit_config = terminal_bench_cfg.get("rate_limit", None)
self._rate_limiter = create_rate_limiter(rate_limit_config)Later, in terminal_bench_agent_loop at line 166-169, the template (still containing rate_limit) is deep-copied and passed to Harbor:
config = deepcopy(self._harbor_config_template)
config["task"] = {"path": prompt}
config["agent"]["kwargs"]["session_id"] = uuid4().hex
trial_config = TrialConfig.model_validate(config)If Harbor's TrialConfig Pydantic model is configured with extra='forbid' (a common setting for strict config validation), this will raise a ValidationError on every trial, causing all trajectories to fail. Even with extra='ignore', passing unrelated config to an external library's validator is unintended and fragile.
Impact: When a user enables rate limiting (the primary feature of this PR), every trial could fail with a Pydantic validation error, making the rate limiting feature unusable.
| # Initialize rate limiter | |
| rate_limit_config = terminal_bench_cfg.get("rate_limit", None) | |
| self._rate_limiter = create_rate_limiter(rate_limit_config) | |
| # Initialize rate limiter | |
| rate_limit_config = terminal_bench_cfg.get("rate_limit", None) | |
| self._rate_limiter = create_rate_limiter(rate_limit_config) | |
| self._harbor_config_template.pop("rate_limit", None) | |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
We add a
rate_limiter.pyimplementation that supports "N trajectories/second" rate throttling and maximum concurrent trajectories throttling.This may be needed when the environment (Daytona/Modal in Harbor's case) might have restrictions on the maximum number of sandbox running concurrently, or the sandbox creation rate; or we'd want smaller pressure on the inference engine (so that there's not much KV eviction happening)
We put the
rate_limiter.pyin a general util folder since it can be compatible with other custom generators.