Reduce pytest warnings to zero #2707

pseudo-rnd-thoughts · 2022-03-19T16:20:23Z

As I noted in #2602, it would be nice if the pytest warnings were thrown as errors so if new changes caused issues
Building upon #2602 that updated the CI and #2703 that fixed warnings (but hasn't been PR yet)

This fixes all warnings from #2660, mostly through added filterwarnings to prevent the warnings from being thrown

- Remove `lint_python` CI job - Move `pyupgrade` job to `pre-commit` workflow

…er.py by using the self._frames_per_sec value as default value if video.frames_per_second is missing

… video.output_frames_per_seconds are the updated frames_per_sec and output_frames_per_sec values

…o errors

…ime steps (this happened with CartPole), if step is done then reset the environment _check_box_obs, fixed spelling of maxmimum to maximum for ignoring the warning

…space that throw a warning to ignore them

…that produce a single element (not an array as contains expected)

kir0ul · 2022-03-21T04:10:54Z

I'm not sure it's a good idea to transform warnings into errors, I think warnings and errors are two different things and having the flexibility of allowing warnings to pass the CI can sometimes be a good thing.
One concrete example could be that someone is working on a feature and gets a deprecation warning. Since this warning has become an error, CI will not pass, and they'll have to fix the warning to make it pass. But maybe this deprecation warning should be fixed in its own dedicated issue as it could impact other things which are independent of the feature they're working on.
I think we should tend to zero warning, but we should allow warnings to pass the CI in case there's the need for some flexibility.

pseudo-rnd-thoughts · 2022-03-21T12:41:10Z

Thanks for the comment. I agree with you but in your example, couldn't they just create a separate PR to address the DeprecatedWarnings? Do you think we should only be throwing errors for user warnings?
I included DeprecationWarnings because when I was doing #2660 a majority of the warnings I was getting were DeprecationWarnings and can be the most critical if left unchanged for a long period of time.

pseudo-rnd-thoughts · 2022-03-21T12:42:38Z

Im setting this as a draft because it requires #2602 and #2703 to be merged first

# Conflicts: # .pre-commit-config.yaml # CONTRIBUTING.md # gym/envs/classic_control/acrobot.py # gym/envs/toy_text/blackjack.py # gym/envs/toy_text/frozen_lake.py # gym/wrappers/__init__.py

…pytest-warnings-to-errors # Conflicts: # gym/envs/toy_text/blackjack.py

… from the low and high array dtypes

pseudo-rnd-thoughts · 2022-04-03T15:32:50Z

With #2602 and #2703 merged, the PR is ready for review @jkterry1
Would this PR be of interest? It should help prevent warnings being thrown and not being fixed

kir0ul · 2022-04-06T16:09:02Z

I agree with you but in your example, couldn't they just create a separate PR to address the DeprecatedWarnings?

Yes they could, but maybe they're working on something unrelated to this warning, which could be fixed in its own specific PR.

Do you think we should only be throwing errors for user warnings? I included DeprecationWarnings because when I was doing #2660 a majority of the warnings I was getting were DeprecationWarnings and can be the most critical if left unchanged for a long period of time.

IMHO I don't think we should force any warnings to become errors. I agree those are annoying but I think the situation you're referring to was different as we got all those warnings at once because of a bug in pytest-forked. Usually warnings would grow slowly and can be corrected as they appear. Of course we should fix all those DeprecationWarning but we can also live with a DeprecationWarning for a short amount of time if for any reason we need time to fix it.
As another example, let's say we find another bug and get another time 1500 warnings all at once, if they are now converted to errors you wouldn't have been able to fix them in several chunks as you did.

Also I don't know if you saw that but there's now a checkbox in the PR template so that developers keep in mind to check for new warnings.

pseudo-rnd-thoughts · 2022-04-06T16:47:37Z

Thanks for the feedback @kir0ul, I agree that this is an important PR that we should consider carefully before merging and I think it would be reasonable not to do so. However, I think there are several important advantages to it

maybe they're working on something unrelated to this warning, which could be fixed in its own specific PR.

I agree with this however, if it is a separate issue, then it would naturally make sense for another PR to make such changes. Or if necessary I believe a force merge could ignore the failing tests

As well, as this commit, sets the number of warnings to zero, then any new warnings that arise should be caused by the changes made by the PR. Meaning that the PR would address their own issues.

This is one of the advantages of stopping warnings early is that the person causing the issue, also has to solve them. Rather than waiting for a while before fixing, by possibly someone new who may misunderstand the original code or the original author who may have forgotten their implementation.

let's say we find another bug and get another time 1500 warnings all at once, if they are now converted to errors you wouldn't have been able to fix them in several chunks as you did.

This is fair, but as I explained above, I think that is very unlikely get ~1500 warnings again, due to eliminating them as they occur and that they would only have to address the warnings caused by themselves most likely.

warning checkbox in the PR template

I hadn't noticed that changes, thanks for doing that, the problem is that the github build actions don't show the number of warnings so it is easy to forget to check if the person has removed warnings. Allowing the number of warnings to increase again, causing the issue above.

Is there something I missing? If this is too radical a change to make then I can understand if we don't want to move forward with this PR

jkterry1 · 2022-04-11T19:52:25Z

@pseudo-rnd-thoughts could you please fix merge conflicts?

…e, the function needs to be moved

…o warnings were produced, then we can just remove this

pseudo-rnd-thoughts · 2022-04-13T23:20:12Z

@kir0ul Talking to @jkterry1, while I still think that the idea of reducing warnings is good, this method of doing it could cause too many issues to be helpful.
I have removed the warnings to errors part and removed some additional warnings that I found such that the number of warnings should be now 0!

jkterry1 · 2022-04-15T14:59:20Z

Hey, I'm closing this per our discussion, please create a new PR with the changes that are still needed

kir0ul and others added 23 commits March 15, 2022 19:03

feat: add isort to pre-commit

f4ffb7f

ci: skip __init__.py file for isort

505930c

ci: make isort mandatory in lint pipeline

cd351bd

docs: add a section on Git hooks

c938aeb

ci: check isort diff

2cbca9b

fix: isort from master branch

eee760d

docs: add pre-commit badge

dc45814

ci: update black + bandit versions

f760a96

feat: add PR template

3784322

refactor: PR template

62c4670

ci: remove bandit

6e97b52

docs: add Black badge

8a7fde5

ci: try to remove all || true statements

674224f

ci: remove lint_python job

1d8533a

- Remove `lint_python` CI job - Move `pyupgrade` job to `pre-commit` workflow

fix: avoid messing with typing

5eef119

ci: apply pre-commit to the whole codebase

449d55b

Fixed warnings produced by test_record_video.py and test_video_record…

406502f

…er.py by using the self._frames_per_sec value as default value if video.frames_per_second is missing

The default backward compatible values if video.frames_per_second and…

175fb13

… video.output_frames_per_seconds are the updated frames_per_sec and output_frames_per_sec values

Merge branch 'video_recorder_warnings' into pytest-warnings-to-errors

cb9db2f

Changed user warnings and deprecation warnings thrown during pytest t…

08774f0

…o errors

_check_nan can cause issues if the environment terminates within 10 t…

5ba9509

…ime steps (this happened with CartPole), if step is done then reset the environment _check_box_obs, fixed spelling of maxmimum to maximum for ignoring the warning

Added filterwarnings for lunar lander and bipedal walker observation …

7254a65

…space that throw a warning to ignore them

Added filterwarnings for iterators that produce a warning for spaces …

80795dd

…that produce a single element (not an array as contains expected)

pseudo-rnd-thoughts mentioned this pull request Mar 19, 2022

Improve pre-commit workflow #2602

Merged

9 tasks

pseudo-rnd-thoughts marked this pull request as draft March 21, 2022 12:41

pseudo-rnd-thoughts added 2 commits April 2, 2022 12:59

Merge branch 'master' into pytest-warnings-to-errors

b47ceef

# Conflicts: # .pre-commit-config.yaml # CONTRIBUTING.md # gym/envs/classic_control/acrobot.py # gym/envs/toy_text/blackjack.py # gym/envs/toy_text/frozen_lake.py # gym/wrappers/__init__.py

Merge branch 'master' into pytest-warnings-to-errors

ee6ea53

# Conflicts: # .pre-commit-config.yaml # CONTRIBUTING.md # gym/envs/classic_control/acrobot.py # gym/envs/toy_text/blackjack.py # gym/envs/toy_text/frozen_lake.py # gym/wrappers/__init__.py

pseudo-rnd-thoughts added 2 commits April 2, 2022 14:57

Merge remote-tracking branch 'origin/pytest-warnings-to-errors' into …

1918de2

…pytest-warnings-to-errors # Conflicts: # gym/envs/toy_text/blackjack.py

Fixed warnings produced by gym.space.Box where the dtype is different…

bd3a98f

… from the low and high array dtypes

pseudo-rnd-thoughts marked this pull request as ready for review April 2, 2022 15:08

Fixed black issues

11f1a1c

pseudo-rnd-thoughts added 5 commits April 12, 2022 13:24

Merge branch 'master' into pytest-warnings-to-errors

e536ed4

On MacOS, processes require the write function to be pickled therefor…

3155e95

…e, the function needs to be moved

pytest.warns(None) is deprecated, as warns None seemed to test that n…

54779cc

…o warnings were produced, then we can just remove this

Rename test files to fit the standard of test_"test subject".py

9e4a9f1

Remove warnings converted to errors

9880056

pseudo-rnd-thoughts changed the title ~~Pytest warnings to errors~~ Reduce pytest warnings to zero Apr 13, 2022

pseudo-rnd-thoughts mentioned this pull request Apr 14, 2022

[Proposal] Add test coverage #2734

Open

1 task

jkterry1 closed this Apr 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce pytest warnings to zero #2707

Reduce pytest warnings to zero #2707

pseudo-rnd-thoughts commented Mar 19, 2022

kir0ul commented Mar 21, 2022

pseudo-rnd-thoughts commented Mar 21, 2022

pseudo-rnd-thoughts commented Mar 21, 2022

pseudo-rnd-thoughts commented Apr 3, 2022

kir0ul commented Apr 6, 2022

pseudo-rnd-thoughts commented Apr 6, 2022 •

edited

Loading

jkterry1 commented Apr 11, 2022

pseudo-rnd-thoughts commented Apr 13, 2022

jkterry1 commented Apr 15, 2022

Reduce pytest warnings to zero #2707

Reduce pytest warnings to zero #2707

Conversation

pseudo-rnd-thoughts commented Mar 19, 2022

kir0ul commented Mar 21, 2022

pseudo-rnd-thoughts commented Mar 21, 2022

pseudo-rnd-thoughts commented Mar 21, 2022

pseudo-rnd-thoughts commented Apr 3, 2022

kir0ul commented Apr 6, 2022

pseudo-rnd-thoughts commented Apr 6, 2022 • edited Loading

jkterry1 commented Apr 11, 2022

pseudo-rnd-thoughts commented Apr 13, 2022

jkterry1 commented Apr 15, 2022

pseudo-rnd-thoughts commented Apr 6, 2022 •

edited

Loading