Deepscholar-Bench#240
Conversation
|
@undfined this is ready for a review when you get a chance |
undfined
left a comment
There was a problem hiding this comment.
Looks good! A couple considerations.
| ds_args = DeepScholarArgs.from_dict(args) | ||
| all_output: list[str] = [] | ||
|
|
||
| provider_url = getattr(provider, "base_url", None) or "http://localhost:8000/v1" |
There was a problem hiding this comment.
This fallback makes API-backed providers without a base_url look like local providers. For example, LiteLLMProvider("gpt-4o") gets provider_url = "http://localhost:8000/v1", so _is_local_provider() returns true and the run fails the local sandbox health check before generation. I think that contradicts the comment below that external API models should keep upstream behavior. Can we distinguish “missing base_url” from “local default URL” before calling _is_local_provider()?
There was a problem hiding this comment.
Fixed by deciding locality from the provider's actual base_url before the local fallback, so a provider without one stays external:
base_url = getattr(provider, "base_url", None)
is_local = self._is_local_provider(provider, base_url or "")
provider_url = base_url or "http://localhost:8000/v1"
Now LiteLLMProvider("gpt-4o") is treated as external (agentic search, no health check) while local vLLM still resolves as local. VLLMServerProvider always assigns self.base_url (passed URL or its managed server's), and passing "" rather than None keeps the _server check intact for a managed provider that hasn't set one yet.
Added a regression test covering all three states. Note the same pattern lives in asta/eval.py and tau2/eval.py; leaving those for a separate follow-up to keep this PR scoped and on track to merge quickly.
| # Hard wall-clock budget for a single S2 search across retries. The full validation | ||
| # encountered frequent 429 responses, so each search needs bounded backoff rather | ||
| # than retrying indefinitely. | ||
| S2_SEARCH_BUDGET_S = 45.0 |
There was a problem hiding this comment.
Nit: _SEC is a bit better
There was a problem hiding this comment.
Renamed to S2_SEARCH_BUDGET_SEC, and the budget_s param to budget_sec for consistency.
|
found a small formatting error for one of the sub-benchmarks, holding off on merging until that's added |
…A/B outputs and normalized permutation scoring
… metrics now fail the evaluation
Description
Adds DeepScholar-Bench as a sandboxed external evaluation (
deepscholar_bench). The benchmark retrieves prior academic work, generates a related-work section, and evaluates it using seven metrics covering organization, coverage, importance, and citation quality.What changed
Validation
Successful full 63-query-range Beaker runs:
01KXEDME5H6SPB432WBM7XE5ES01KXEVZVCJHQMCK336MDZHD5HXLocal validation:
uv run pytest -q tests/core/harness/test_sandbox_executor.py tests/evals/external/benchmarks/test_deepscholar_external.py— 18passed
<add experiment ID>The streaming regression test verifies both sides of the poll behavior: an in-progress command may return exit code 1 with a valid marker, while a container failure without the marker reaches the existing three-strike abort.
Type of Change
Testing
Checklist