Skip to content

fix(core): reject unsupported TaskProtocol retry settings - #66

Open
kingpy-bot wants to merge 3 commits into
maseval:mainfrom
kingpy-bot:kingpy/fail-fast-unsupported-task-retries-62
Open

fix(core): reject unsupported TaskProtocol retry settings#66
kingpy-bot wants to merge 3 commits into
maseval:mainfrom
kingpy-bot:kingpy/fail-fast-unsupported-task-retries-62

Conversation

@kingpy-bot

Copy link
Copy Markdown

Closes #62.

Summary

  • Document TimeoutAction.RETRY / EXTEND and TaskProtocol.max_retries as reserved until execution-loop retry handling exists
  • Add TaskProtocol.__post_init__ validation so unsupported retry settings fail loudly instead of being silently ignored
  • Update protocol tests to cover the supported defaults plus rejection of reserved retry settings

Tests

  • .venv/bin/python -m pytest tests/test_core/test_task_protocol.py tests/test_core/test_benchmark/test_report_schema.py -q
  • .venv/bin/python -m pytest tests/test_core -q
  • .venv/bin/ruff check maseval/core/task.py tests/test_core/test_task_protocol.py
  • .venv/bin/ruff format --check maseval/core/task.py tests/test_core/test_task_protocol.py
  • .venv/bin/ty check maseval/core/task.py tests/test_core/test_task_protocol.py
  • git diff --check

AI-assisted contribution: prepared with an AI coding assistant and manually verified with the commands above.

Widen test_max_retries_is_rejected to a parametrized test that covers negative, small, and large non-zero max_retries values, not just max_retries=1.

The docstring contract is 'max_retries must currently be 0', and the validation should reject every non-zero int, not just the smallest one.

Refs: maseval#66
@Billyzhao007

Copy link
Copy Markdown

Thanks for taking the fail-loud route here. I reproduced the original silent no-op and reviewed this branch locally. The core suite passes, but I found two gaps that appear blocking before merge:

  1. Tau2 and GAIA2 both still default DEFAULT_MAX_RETRIES = 1, so their normal task conversion now raises ValueError under this PR.
  2. TaskProtocol remains mutable. protocol = TaskProtocol(); protocol.max_retries = 2 bypasses __post_init__, and Benchmark.run() still executes once while reporting max_retries: 2.

A minimal completion could keep the current constructor validation, add execution-boundary validation before callbacks/workers are started, change the two built-in benchmark defaults and docs to 0, and add offline converter regressions for Tau2/GAIA2. I have deterministic local reproductions for both cases and can help prepare the follow-up if useful.

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.

TaskProtocol.max_retries and TimeoutAction.RETRY/EXTEND are declared but never consumed

2 participants