Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 26, 2025

closes #1251

Summary

Fixed all linting, type checking, and documentation build issues while improving the VecEnv checker implementation.

✅ Completed Fixes

Code Quality:

  • [x] Fixed all linting issues - Resolved RUF005 warnings by using tuple unpacking syntax
  • [x] Fixed type checking issues - Added type ignore annotation for compatibility
  • [x] Fixed documentation build - Corrected title underline length issue
  • [x] Reduced code complexity - Simplified _check_vecenv_unsupported_spaces by reusing existing function
  • [x] Removed unused imports - Cleaned up import statements

Error Messages:

  • [x] Improved error messages with actual values - Added f-strings with actual types and values:
    # Before:
    assert isinstance(vec_env.num_envs, int), "num_envs must be a positive integer"
    
    # After: 
    assert isinstance(vec_env.num_envs, int), f"num_envs must be a positive integer, got {vec_env.num_envs} (type: {type(vec_env.num_envs)})"

Code Reuse:

  • [x] Reused methods from env_checker - Now imports and reuses:
    • _check_box_obs for observation space validation
    • _check_unsupported_spaces for space compatibility warnings
  • [x] Removed code duplication - Eliminated duplicate helper functions

Exception Handling:

  • [x] Removed broad exception catching - Eliminated try/except Exception blocks as requested
  • [x] Let method calls fail naturally - Functions now propagate exceptions appropriately

Linting Results:

  • All ruff checks pass ✅
  • All mypy type checks pass ✅
  • All tests pass (6 VecEnv tests + 25 original tests) ✅
  • Documentation builds without warnings ✅

The VecEnv checker now provides better error messages, reuses existing code properly, and follows all project coding standards.

Original prompt

Similar to the env_checker in SB3 for gymnasium env, create a VecEnv checker for SB3 VecEnv (see issue #1251), don't forget to add tests and update the changelog (details are in the contributing guide).


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@Copilot Copilot AI changed the title [WIP] Similar to the env_checker in SB3 for gymnasium env, create a VecEnv checker for SB3 VecEnv (see issue #1251), don't forget to add tests and update the changelog (details are in the contributing guide). Add check_vecenv function for VecEnv validation similar to env_checker Sep 26, 2025
@Copilot Copilot AI requested a review from araffin September 26, 2025 11:48
Copilot finished work on behalf of araffin September 26, 2025 11:48
@araffin
Copy link
Member

araffin commented Sep 26, 2025

@copilot the env checker file is too long, put the vec env checker code in another file, same for the tests.
Update also the documentation to add an example on how to use/when to use the vec env checker.

Copy link
Contributor Author

Copilot AI commented Sep 26, 2025

@copilot the env checker file is too long, put the vec env checker code in another file, same for the tests.
Update also the documentation to add an example on how to use/when to use the vec env chec...

Done! Refactored the VecEnv checker implementation to address all your feedback:

Code Organization:

  • Moved VecEnv checker code to new dedicated stable_baselines3/common/vec_env_checker.py file
  • Moved VecEnv tests to new dedicated tests/test_vec_env_checker.py file
  • Resolved circular imports by duplicating needed helper functions
  • Made check_vecenv accessible via stable_baselines3.common.vec_env import

Documentation:

  • Added comprehensive documentation in docs/guide/vec_envs.rst with usage examples and guidance on when to use the checker
  • Included complete code samples showing how to validate custom VecEnv implementations

All functionality preserved, all tests pass (6 new VecEnv tests + 25 original env_checker tests). The env_checker.py file is now back to its original length without the VecEnv code.

Commit: 005ab7f

Copilot finished work on behalf of araffin September 26, 2025 12:29
@araffin
Copy link
Member

araffin commented Sep 26, 2025

@copilot check that the proposes changes do not raise any new issues from the linter/type checker (use pip install -e .[tests,docs] and then make commit-checks), fix the build issue is the doc (make doc), and re-use methods from the env checker inside the vec env checker when possible.

Whenever possible, give more information about expected type to the user, for instance assert isinstance(vec_env.num_envs, int) and vec_env.num_envs > 0, "num_envs must be a positive integer" need to be replaced with assert isinstance(vec_env.num_envs, int) and vec_env.num_envs > 0, f"num_envs must be a positive integer, not {vec_env.num_envs}"

Do not use except Exception as e:, it's ok if the method calls fails.

Copy link
Contributor Author

Copilot AI commented Sep 26, 2025

@copilot check that the proposes changes do not raise any new issues from the linter/type checker (use pip install -e .[tests,docs] and then make commit-checks), fix the build issue is the doc (`m...

Done! Fixed all linting, type checking, and documentation build issues:

Linting & Type Checking:
✅ All ruff checks pass
✅ All mypy type checks pass
✅ Documentation builds successfully

Improvements Made:

  • Better error messages - Now include actual values: f"num_envs must be a positive integer, got {vec_env.num_envs} (type: {type(vec_env.num_envs)})"
  • Code reuse - Imports and reuses _check_box_obs and _check_unsupported_spaces from env_checker instead of duplicating code
  • No broad exceptions - Removed all try/except Exception blocks, letting method calls fail naturally
  • Reduced complexity - Simplified _check_vecenv_unsupported_spaces by delegating to existing function
  • Fixed tuple concatenation - Used unpacking syntax (vec_env.num_envs, *space.shape) instead of concatenation
  • Fixed regex warnings - Added raw strings for regex patterns

All tests still pass (6 VecEnv tests + 25 original env_checker tests). The implementation now follows all project standards.

Commit: ff2c000

Copilot finished work on behalf of araffin September 26, 2025 12:56
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.

[Feature Request] Env checker for VecEnv
2 participants