Replace (part of) pretest of CI with Pre-commit equivalent #918

lebrice · 2022-05-18T20:43:20Z

Addresses #918

Also applies the formatting, spelling mistakes, and python upgrade hooks to all files.

lebrice · 2022-05-19T00:00:02Z

Hmm this doesnt quite work: seems like it's only running the pre-commit workflow at the moment.

bouthilx · 2022-05-19T17:09:26Z

Is there a corrector is the pre-commit? 😯 That's great!

It seems the pre-commit is missing the doc compilation. That's to long to be included in the pre-commit indeed but should still be part of the pre-tests.

bouthilx · 2022-05-19T17:11:20Z

There also seems to be no equivalent for pylint in the pre-commit.

lebrice · 2022-05-19T19:01:50Z

Ok @bouthilx I left the doc and pylint steps as part of pre-test, and moved the rest to pre-commit. Pylint is a bit annoying to use as a pre-commit hook, since it requires having pylint installed locally. It also seems to take a really long time, from my limited experience.

bouthilx · 2022-05-20T20:30:10Z

.github/workflows/build.yml

  pretest:
+    needs: pre-commit


Can we not wait for pre-commit? If we could set the execution of pre-commit in tox we could have toxenv: [pylint, docs, pre-commit] so that the main tests still depends on the same pretests.

The main tests still depend on the same pretests though, right?
I dont quite understand.

What would be the advantage of running pre-commit through tox vs running pre-commit as a github action with the same jobs?

Running them through tox would make it possible to include the pre-commit tests in the current pre-test matrix. Having the pre-commit tests available in the tox config would also make it possible to run these tests locally if necessary outside of the pre-commit hook.

Note: The tests can be run locally with pre-commit run <hook_name>, and either --all-files, or --files <files>.

bouthilx · 2022-06-27T15:47:43Z

What is blocking this PR at the moment?

lebrice · 2022-06-28T05:13:18Z

As far as I recall, I need to basically fix a few tests that got broken by removing unused imports in all modules.

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

I was previously adding some more hooks. I'll instead keep those locally, but only replicate the current CI setup with pre-commit. Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

This removes the need to import fixtures as unused variables in the other test packages. The autoflake hook removed those unused imports, and with this, we don't need to import them in the first place. Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

bouthilx · 2022-07-06T17:33:39Z

src/orion/benchmark/task/profet/model_utils.py

@@ -219,7 +219,7 @@ def load_data(
                raise RuntimeError(
                    f"Download finished, but file {file} still doesn't exist!"
                )
-        with open(file, "r") as f:
+        with open(file) as f:


Doesn't pylint complain about not specifying the encoding? 🤔

bouthilx

This will be awesome to use, thanks a lot!

lebrice marked this pull request as ready for review May 18, 2022 23:57

lebrice changed the title ~~Replace pretest of CI with Pre-commit equivalent~~ Replace (part of) pretest of CI with Pre-commit equivalent May 19, 2022

bouthilx reviewed May 20, 2022

View reviewed changes

lebrice and others added 21 commits July 4, 2022 17:58

Run pre-commit on all files, fix spelling mistakes

759abd4

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

Fix typos pointed to by codespell

a6edc75

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

Create pre-commit github-actions workflow

a446945

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

Replace pretest with pre-commit

f791f0b

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

Fix ALL flake8 errors in the entire repo

09bd3e3

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

Fix all pre-commit issues

3f32a43

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

Modify build.yml, remove pre-commit.yaml

1e8f5dd

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

Only replicate current CI with pre-commit (desc)

e25a59f

I was previously adding some more hooks. I'll instead keep those locally, but only replicate the current CI setup with pre-commit. Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

Correct module docstring

1eae2d4

Signed-off-by: Fabrice Normandin <normandf@mila.quebec>

Make pre-commit part of pretest

de9cb96

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Pre-commit as parallel step to pretest

cbb7803

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix missing brackets

02aff91

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Remove matrix entry for pre-commit hooks

1dcb5f3

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix black issue in docs/conf.py

468c9d2

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix doc8 issue in dashboard/src/README.rst

b027b42

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix pre-commit errors in all files for all hooks

6cf14c0

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix import issue in database tests

4e316ad

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Ignore dashboard build dir, re-apply all hooks

4eb2ef3

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Use tox for doc8, replace other jobs w pre-commit

8f3a6dc

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix name resolve error in test_io.py

06f7085

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix trailing whitespace issues in test_info.py

ec2e440

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

lebrice force-pushed the pre-commit-ci branch from 0a2f563 to ec2e440 Compare July 4, 2022 22:09

Fix leftover merge conflicts in build.yml

bb5c8b1

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

lebrice force-pushed the pre-commit-ci branch from 53e2fd9 to bb5c8b1 Compare July 4, 2022 22:13

lebrice added 6 commits July 4, 2022 18:16

Fix typos

411efe2

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Remove redundant 'r' mode (from pre-commit hook)

b1801db

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Disable doc8 pre-commit hook in CI

ff64f96

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Re-add removed portion of build.yml

52bc0b5

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Remove black and isort tox envs, update pre-commit

362e9c5

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

Fix broken dependency in lint tox job

b99203c

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>

bouthilx reviewed Jul 6, 2022

View reviewed changes

bouthilx approved these changes Jul 6, 2022

View reviewed changes

lebrice merged commit 5fb2b4e into Epistimio:develop Jul 6, 2022

lebrice deleted the pre-commit-ci branch July 6, 2022 18:51

bouthilx added the ci label Jul 6, 2022

notoraptor mentioned this pull request Aug 2, 2022

Release 0.2.5rc #980

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace (part of) pretest of CI with Pre-commit equivalent #918

Replace (part of) pretest of CI with Pre-commit equivalent #918

lebrice commented May 18, 2022 •

edited

Loading

lebrice commented May 19, 2022 •

edited

Loading

bouthilx commented May 19, 2022

bouthilx commented May 19, 2022

lebrice commented May 19, 2022

bouthilx May 20, 2022

lebrice May 26, 2022

lebrice May 26, 2022 •

edited

Loading

bouthilx May 26, 2022

lebrice May 31, 2022

bouthilx commented Jun 27, 2022

lebrice commented Jun 28, 2022

bouthilx Jul 6, 2022

bouthilx left a comment

Replace (part of) pretest of CI with Pre-commit equivalent #918

Replace (part of) pretest of CI with Pre-commit equivalent #918

Conversation

lebrice commented May 18, 2022 • edited Loading

lebrice commented May 19, 2022 • edited Loading

bouthilx commented May 19, 2022

bouthilx commented May 19, 2022

lebrice commented May 19, 2022

bouthilx May 20, 2022

Choose a reason for hiding this comment

lebrice May 26, 2022

Choose a reason for hiding this comment

lebrice May 26, 2022 • edited Loading

Choose a reason for hiding this comment

bouthilx May 26, 2022

Choose a reason for hiding this comment

lebrice May 31, 2022

Choose a reason for hiding this comment

bouthilx commented Jun 27, 2022

lebrice commented Jun 28, 2022

bouthilx Jul 6, 2022

Choose a reason for hiding this comment

bouthilx left a comment

Choose a reason for hiding this comment

lebrice commented May 18, 2022 •

edited

Loading

lebrice commented May 19, 2022 •

edited

Loading

lebrice May 26, 2022 •

edited

Loading