Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Remove eval_env parameter #925

Closed
1 task done
araffin opened this issue May 29, 2022 · 2 comments · Fixed by #1104
Closed
1 task done

[Feature Request] Remove eval_env parameter #925

araffin opened this issue May 29, 2022 · 2 comments · Fixed by #1104
Labels
enhancement New feature or request

Comments

@araffin
Copy link
Member

araffin commented May 29, 2022

🚀 Feature

eval_env and eval_freq parameters were new in SB3 and were meant to facilitate evaluation in a separate env, by automatically creating evaluation env and EvalCallback.
However, they don't seem to be used that much and don't offer the flexibility of the EvalCallback at the cost of code duplication and maybe confusing parameters.

The idea would be to first deprecate (next SB3 version) and then remove all the parameters related to that feature.
Main drawback is that this would be a breaking change for people implementing custom algorithms.

### Checklist

  • I have checked that there is no similar issue in the repo (required)
@tobirohrer
Copy link
Contributor

Hey,
I wanted to check if this change is still planned. If so, I would take a look at it in the coming days.

@araffin
Copy link
Member Author

araffin commented Sep 19, 2022

Hello,
it is still planned, but it's not a priority.
I would like to integrate LN and dropout (#1069), and probably progress bar callback first (DLR-RM/rl-baselines3-zoo#287).
Actually, I would welcome more help to improve the RL Zoo.

However, you can of course work on such PR, please only deprecate the feature first (it should output a warning and remove the feature).

tobirohrer added a commit to tobirohrer/stable-baselines3 that referenced this issue Sep 30, 2022
qgallouedec added a commit that referenced this issue Oct 10, 2022
)

* Adds deprecation warning if `eval_env` or `eval_freq` parameters are used. See #925

* added changelog entry

* added missing backtick

* deprecating `create_eval_env` parameter as well and adding comments to explain the `stacklevel` parameter used

* Updated tests to ignore DeprecationWarnings

* Updated changelog entry

* - Removed the `create_eval_env` parameter from the examples in the docs
- Removed information about the `create_eval_env` parameter from the migration docs
- Added information about deprecation of the `create_eval_env` parameter in the docs

* Add alternative in docstring

* Update docstrings

* `eval_freq` warning in docstring

* Add deprecation comments in tests

Co-authored-by: Quentin Gallouédec <[email protected]>
Co-authored-by: Antonin RAFFIN <[email protected]>
Co-authored-by: Quentin GALLOUÉDEC <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants