Improve pre-commit workflow

[kir0ul]

This PR will add the following changes:

• apply isort to the whole codebase,
• activate isort in CI (at the moment it can never fail)
• add the isort check to the pre-commit file so that developers can know beforehand and not after pushing if something is wrong with their imports

EDIT:

• update black version used by pre-commit
• add a PR template
• describe the pre-commit workflow in the CONTRIBUTING.md file

EDIT2:

• remove the bandit job in pre-commit
• remove the lint_python CI job
• move the existing pyupgrade job to pre-commit

[kir0ul]

Note: I'm not completely sure why but I wasn't able to sort the imports of gym/wrappers/__init__.py and gym/__init__.py, so as a workaround I just skipped those files.

[jkterry1]

@kir0ul this looks good, but could you fix this in CI and and fix merge conflicts?

[kir0ul]

I believe this is ready for review @jkterry1 🙂
I was having a lot of issues with the tests in CI I couldn't reproduce on my local machine, so I restarted the branch from scratch and had to work inside the Docker container from the CI to reproduce the issues, anyway it seems to work now! 🎉
In the end I limited the files skipped by isort to only one file: gym/__init__.py.

[RedTachyon]

No issues with sorting the imports in the codebase, it's about as boring and uncontroversial as it gets.

My bigger question is - what exactly is the implication on the process of submitting new PRs? As I understand, CI will fail if imports are wrong? What will contributors have to do to make it pass? This should be clearly stated somewhere.

I'm also personally not a fan of requiring the isort and failing tests otherwise, imo it doesn't contribute much to code quality, and might be an extra annoyance of failing tests for relatively trivial reasons.

[kir0ul]

My bigger question is - what exactly is the implication on the process of submitting new PRs? As I understand, CI will fail if imports are wrong? What will contributors have to do to make it pass? This should be clearly stated somewhere.

Yes at the moment the CI will fail if imports are not sorted, but if the person has pre-commit installed, they will know right at commit time that the CI will fail. Committing (with pre-commit installed) or running pre-commit manually will sort the imports for them.
I agree it would be nice to document this. I guess it's not specific to isort but more about all the pre-commit pipelines in general which, as far as I could find, is not documented anywhere. I could see adding a paragraph about pre-commit in the CONTRIBUTING.md docs.

[jkterry1]

@kir0ul could you update this PR with a description in contributing.md?

[kir0ul]

I added a section in the CONTRIBUTING.md file.

[kir0ul]

I updated the list of changes this PR makes in the first post: #2602 (comment)

[RedTachyon]

I think it looks good now, if my understanding is right, the workflow for formatting should just be running pre-commit and then forget about it?

Overall I'm happy with this, can be merged imo

[kir0ul]

I think it looks good now, if my understanding is right, the workflow for formatting should just be running pre-commit and then forget about it?

Yes, it should indeed run automatically when you make a commit if you installed the Git hooks with pre-commit install.

[kir0ul]

General question before merging: is anyone having any issue with bandit when running it locally?

For some reason it doesn't work on my local machine, like it takes forever and never finishes. So I'm wondering if it's only a problem for me or also for others. Because in case it's not only a problem on my machine and people start to use it with pre-commit, then I'm worried everyone gets the same issue as I have.

[RedTachyon]

Do we actually need bandit to be run in the first place? I'm not really familiar with it, and it seems to be doing some sort of a security audit, so my intuition is that it's enough if it runs in the CI, and doesn't need to be ran locally

[jkterry1]

I think I said this to you over discord, but I've had a ton of issues with bandit. Given that gym isn't an online service, I feel like removing it from CI outright may be prudent? Also can you please fix the merge conflict?

[kir0ul]

I removed Bandit completely and fixed the merge conflicts.

While doing this, I was also looking at the lint_python job in the lint_python action, which seems to be useless as isort already runs in pre-commit, and everything else it runs can never fail:

  lint_python:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-python@v2
      - run: pip install isort pytest pyupgrade safety
      - run: isort --diff --check-only --profile black .
      - run: pip install -e .[nomujoco]
      - run: pytest . || true
      - run: pytest --doctest-modules . || true
      - run: shopt -s globstar && pyupgrade --py36-plus **/*.py || true

Is there a reason to keep this or should it be removed/commented?

[jkterry1]

"everything else it runs can never fail"

could you please elaborate on that?

[kir0ul]

All the run something || true statements in the lint_python job will always pass even if run something fails.

For example, if I add a test that should always fail:

def test_that_should_fail():
    assert False

If I run the line below I get an exit code of 1, so it fails as expected.

pytest -k test_that_should_fail; echo $?  # returns 1

But if I add the || true statement as below, then I get an exit code of 0, so even if the test fails, the whole statement passes.

pytest -k test_that_should_fail || true; echo $?  # returns 0

[jkterry1]

I understand, I apologize. All of the || true stamps obviously should be removed and the tests should be made to pass?

[kir0ul]

Looking more closely at what is run in the lint_python job:

1. isort --diff --check-only --profile black . : duplicated as isort is already run in the pre-commit pipeline;
2. pytest . : duplicated as tests are already run in the build pipeline;
3. pytest --doctest-modules . : not sure what to do with this one, I think it's good to check the code examples in the docstrings but I wasn't able to make it work, maybe this could be its own separate issue?
4. shopt -s globstar && pyupgrade --py37-plus **/*.py : used to upgrade the code to newer versions of Python, could be moved to the pre-commit pipeline since there's an already existing pre-commit hook.

In summary, I'd suggest to completely remove the lint_python job, move pyupgrade to the pre-commit pipeline, and put pytest --doctest-modules . on hold for now (except if anyone has any hints on how to make it work).

[pseudo-rnd-thoughts]

Could you remove --forked from the tests in build as we found that pytest-forked is bugged #2647

[kir0ul]

Could you remove --forked from the tests in build as we found that pytest-forked is bugged #2647

Yes, I just rebased on master.

• I still need to investigate the typing issues between pyright and pyupgrade.

[kir0ul]

Looks like this is ready. I updated the first post with the content of what this PR does.

[pseudo-rnd-thoughts]

This may be too late to add to this PR but dealing with #2660 and #2703
Is there a way to capture the number of warnings that pytest produces and prevent the check from being passed if "new" warnings as produced by a PR?
The reason I say "new" is because of #2660 I am aware of 12 warnings for which we cannot remove the warnings (at least easily)

[kir0ul]

This may be too late to add to this PR but dealing with #2660 and #2703
Is there a way to capture the number of warnings that pytest produces and prevent the check from being passed if "new" warnings as produced by a PR?
The reason I say "new" is because of #2660 I am aware of 12 warnings for which we cannot remove the warnings (at least easily)

This sounds out of the scope of this PR, but is there no way to ignore/hide only those specific warnings? I didn't investigate much but maybe @pytest.mark.filterwarnings or the catch_warnings() context manager could help?

[pseudo-rnd-thoughts]

@kir0ul Thanks, I used this branch to add that feature in #2707

[RedTachyon]

So just as a final sanity check as to what this does - after installing and configuring pre-commit, all the formatting stuff will be performed on the local copy of the code, and then that reformatted version will be committed?

If that is the case, then it looks alright. (ignoring the merge conflict that happened in the meantime)

[kir0ul]

So just as a final sanity check as to what this does - after installing and configuring pre-commit, all the formatting stuff will be performed on the local copy of the code, and then that reformatted version will be committed?

Correct, and the exact same checks run locally will also be run in CI.
I also noticed you may have to run it a couple of times manually to make it pass as each formatting tool will first format the code and fail at the first time and pass at the second time. I guess I should mention that in the contributing docs.
I'm away from keyboard for a few days but I'll try to make this last change to the docs and fix the conflicts as soon as I get the chance.

[kir0ul]

@RedTachyon Just added a note in the contributing docs & fixed the conflicts!