fix: replace assert-based validation with proper exceptions#420
fix: replace assert-based validation with proper exceptions#420hobostay wants to merge 1 commit into
Conversation
Using assert for input validation is dangerous because assertions are stripped when Python runs with -O (optimize mode), causing silent failures and confusing KeyError tracebacks instead of helpful error messages. Changes: - registration.py: Replace all assert statements with if/raise ValueError for task_id, env_type, num_envs, batch_size, max_num_players, seed, and env_seed validation - utils.py: Fix incorrect SystemError to RuntimeError for duplicate key detection (SystemError is reserved for interpreter-level errors) - envpool.py: Improve close() error handling by distinguishing ImportError from other exceptions and issuing a RuntimeWarning instead of silently swallowing all errors Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d683b91a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raise ValueError("num_envs must be >= 1.") | ||
| if "batch_size" in kwargs: | ||
| assert 0 <= kwargs["batch_size"] <= kwargs["num_envs"] | ||
| if not (0 <= kwargs["batch_size"] <= kwargs["num_envs"]): |
There was a problem hiding this comment.
When a caller supplies batch_size but relies on the default num_envs=1, this replacement now always evaluates kwargs["num_envs"] and raises KeyError before gen_config can apply the common default. This is a regression for the optimized-mode path this change is meant to fix: before this commit, python -O stripped the assert and allowed valid calls such as make_spec(task_id, batch_size=1) to use the default num_envs; use kwargs.get("num_envs", 1) (and the same value in the message) for this validation.
Useful? React with 👍 / 👎.
Trinkle23897
left a comment
Trinkle23897
left a comment
There was a problem hiding this comment.
please fix the test and please don't make a change for change; I don't think this claude diff is valuable
|
Closing per reviewer feedback. The maintainer considers these changes unnecessary churn. The assert-to-raise conversion, while a common Python best practice, is not considered valuable for this codebase, and the PR also introduces a lint failure and breaks CI. |
2 participants
Summary
Problem
Using assert for input validation is a well-known Python anti-pattern. When Python is invoked with -O, all assert statements are completely removed, causing silent failures.
Test plan
🤖 Generated with Claude Code