Remove all files from the pre-commit exclude list #4196

sphuber · 2020-06-23T22:31:35Z

Fixes #2207 and fixes #3885

codecov · 2020-06-24T06:33:28Z

Codecov Report

Merging #4196 into develop will increase coverage by 0.06%.
The diff coverage is 52.60%.

@@             Coverage Diff             @@
##           develop    #4196      +/-   ##
===========================================
+ Coverage    79.11%   79.16%   +0.06%     
===========================================
  Files          467      468       +1     
  Lines        34492    34463      -29     
===========================================
- Hits         27284    27280       -4     
+ Misses        7208     7183      -25

Flag	Coverage Δ
#django	`71.08% <48.82%> (+0.06%)`	⬆️
#sqlalchemy	`71.92% <50.22%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
aiida/backends/djsite/queries.py	`92.79% <ø> (ø)`
aiida/backends/sqlalchemy/migrations/env.py	`80.65% <0.00%> (ø)`
...ons/07fac78e6209_drop_computer_transport_params.py	`100.00% <ø> (ø)`
...ns/12536798d4d3_trajectory_symbols_to_attribute.py	`95.66% <ø> (ø)`
...grations/versions/162b99bca4a2_drop_dbcalcstate.py	`91.67% <ø> (ø)`
...30c8430131_drop_node_columns_nodeversion_public.py	`100.00% <ø> (ø)`
...61acd560_data_migration_legacy_job_calculations.py	`100.00% <ø> (ø)`
...sions/37f3d4882837_make_all_uuid_columns_unique.py	`91.31% <ø> (ø)`
...migrations/versions/5a49629f0d45_dblink_indices.py	`100.00% <ø> (ø)`
...ations/versions/61fc0913fae9_remove_node_prefix.py	`100.00% <ø> (ø)`
... and 85 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52bd5ad...6c251bd. Read the comment docs.

sphuber · 2020-06-24T12:04:04Z

@ramirezfranciscof and @chrisjsewell this bad boy is ready to go!

chrisjsewell

Nice 👌

sphuber · 2020-06-24T12:18:14Z

To anyone reading this Note that this should not be merged!

ltalirz · 2020-06-30T11:04:18Z

That's fantastic @sphuber :-)

One small question: I notice that you moved (in a previous PR) the pylint hook from local to repo: https://github.com/PyCQA/pylint- but then of course you are still using language: system since it needs to see AiiDA and its dependencies.
Just wondering: what are the benefits of this? Does this not install a separate virtual environment with pylint inside that is never used?
I.e. would it not be better to move the pylint hook back under the local ones?

P.S. I'm curious - did removing the exclude list surface any bugs?

sphuber · 2020-06-30T12:10:58Z

That's fantastic @sphuber :-)

Combination of OCD and procrastination is one heck of a drug ;)

One small question: I notice that you moved (in a previous PR) the pylint hook from local to repo: https://github.com/PyCQA/pylint- but then of course you are still using language: system since it needs to see AiiDA and its dependencies.
Just wondering: what are the benefits of this? Does this not install a separate virtual environment with pylint inside that is never used?
I.e. would it not be better to move the pylint hook back under the local ones?

As you said, we need to run this with language: system such that we don't get spurious import-error warnings. However, we also want to run with a very specific version of pylint because changes in that version can cause differences between local and CI environment and therefore annoying CI failures. We used to "solve" this by pinning the requirement in the requirements. However, this would cause problems when developing multiple packages in the same virtual environment. Often there would be version conflicts. So you would run pre-commit with aiida-core and so that would require a specific pylint version, but then aiida-quantumespresso would be still on a different version and so when you commit, pre-commit would fail. By using the remote hook, the separate virtual environment solves these dependency incompatibilities. I think this is worth the little overhead, because anyway the environment is cached.

P.S. I'm curious - did removing the exclude list surface any bugs?

Yes, plenty! Mostly in the data plugins. They are under tested and are some of the oldest code in aiida-core.

ltalirz · 2020-06-30T12:20:08Z

However, we also want to run with a very specific version of pylint because changes in that version can cause differences between local and CI environment and therefore annoying CI failures. We used to "solve" this by pinning the requirement in the requirements.

Perhaps I'm wrong, but I believe the version (and executable) of pylint that you are using is the one installed by the pre-commit extra of aiida-core, not the version installed by the pylint repo.
At least that's how I understand what is written here: https://pre-commit.com/#system

Otherwise, how are they mixing the two python environments?
Or asked the other way around: If it was using the pylint version from the hook repo, then why do we still need it in the pre-commit requirements?

However, this would cause problems when developing multiple packages in the same virtual environment. Often there would be version conflicts

This point I fully support, i.e. I'm all for moving linters, formatters etc. into separate environments where we can.
I'm just not sure it's possible with pylint.

chrisjsewell · 2020-06-30T12:27:24Z

not the version installed by the pylint repo

yeh pretty sure thats the case

This point I fully support, i.e. I'm all for moving linters, formatters etc. into separate environments if we can.

Just in case you didn't know, you can get pre-commit to install additional packages into its environment with: https://pre-commit.com/#config-additional_dependencies. But thats probably not an adequate cure for the import-error warnings

chrisjsewell · 2020-07-01T20:42:13Z

.pre-commit-config.yaml

+-   repo: https://github.com/PyCQA/pylint
+    rev: pylint-2.5.2


were we going to move this into the local repo, as suggested by @ltalirz ?

But then we are back to the situation where we have to pin an exact version which will collide with any other projects you might have in the same virtual env. Is there no way to address this? I am not sure if this current solution actually allowed for this or if it had the same limitation and I just never noticed the collision

I think we still are in this situation - the only difference being that now we create an additional virtual environment that is never used

but as we discussed, I'm under the impression that, because of language: system, it is already using the virtual env version of the package not the one that is installed in the pre-commit env

to use the one in the pre-commit env, you would have to either ignore errors regarding dependency installs, or also install them dependencies into the pre-commit environment, using https://pre-commit.com/#config-additional_dependencies

@chrisjsewell not sure whether you replied to my comment or seb's - anyhow I did mean we create an additional pre-commit environment that is never used, i.e. we are saying the same thing.

As for your following comment, I don't think either of those are viable solutions, i.e. we can just move the hook back under local

Moved it back to local

not sure whether you replied to my comment or seb's

seb's

I don't think either of those are viable solutions

yep agree, just outlining the possible option 👍

Except for the documentation of course, which should remain excluded. Also move the `pylint` pre-commit hook back under `local`. The idea by putting it under the remote repos was to profit from the separate virtual environment that is created with the exact version specified in order to prevent clashes with requirements of other projects being developed in the same virtual environment. However, this approach leads to many spurious false positive import-errors because `pylint` cannot find all other third party dependencies. This can be fixed by specifying `language: system`, but this just forces the normal virtual environment to be used, rendering the whole point of using the remote repos moot.

ltalirz · 2020-07-11T22:47:16Z

.pre-commit-config.yaml

+    hooks:
+    -   id: pylint
+        name: pylint
+        language: system
+        exclude: *exclude_files
+
    hooks:


@sphuber I didn't review this PR but I think you might have just disabled pylint there (hooks key used twice)...

The configured hook doesn't actually work - you'll need to add

entry: pylint

Thanks for the heads-up @ltalirz ! That is indeed a required key but normally that should raise a an error when the file is parsed

An error has occurred: InvalidConfigError: ==> File .pre-commit-config.yaml ==> At Config() ==> At key: repos ==> At Repository(repo='local') ==> At key: hooks ==> At Hook(id='pylint') =====> Missing required key: entry

The bigger problem is that I accidentally defined the hooks key twice. The first one, containing the pylint config simply gets overridden, which is why pylint is never actually run. I will create a PR to fix it.

See PR #4258

The bigger problem is that I accidentally defined the hooks key twice.

Right. That's what I wrote above, no? ;-)

You did, but not in the original post. I read it on my email, which only ever send the first version. So you edit didn't show up Guess I could have noticed when responding here though :)

sphuber requested review from chrisjsewell and ramirezfranciscof June 23, 2020 22:31

sphuber force-pushed the fix/pre-commit-exclude branch from 814ba36 to 4ed8cfc Compare June 24, 2020 06:18

sphuber force-pushed the fix/pre-commit-exclude branch from 4ed8cfc to ccd4140 Compare June 24, 2020 06:38

sphuber added the pr/on-hold PR should not be merged label Jun 24, 2020

sphuber force-pushed the fix/pre-commit-exclude branch 5 times, most recently from 8e2ff02 to e2a183b Compare June 24, 2020 11:39

sphuber changed the title ~~Remove files from the pre-commit exclude list~~ Remove all files from the pre-commit exclude list Jun 24, 2020

chrisjsewell previously approved these changes Jun 24, 2020

View reviewed changes

sphuber dismissed chrisjsewell’s stale review via cd52368 July 1, 2020 15:57

sphuber force-pushed the fix/pre-commit-exclude branch from e2a183b to cd52368 Compare July 1, 2020 15:57

sphuber removed the pr/on-hold PR should not be merged label Jul 1, 2020

sphuber requested a review from chrisjsewell July 1, 2020 20:37

chrisjsewell reviewed Jul 1, 2020

View reviewed changes

sphuber force-pushed the fix/pre-commit-exclude branch from cd52368 to 6c251bd Compare July 1, 2020 21:04

chrisjsewell approved these changes Jul 1, 2020

View reviewed changes

sphuber merged commit 855ae82 into aiidateam:develop Jul 1, 2020

sphuber deleted the fix/pre-commit-exclude branch July 1, 2020 21:20

ltalirz reviewed Jul 11, 2020

View reviewed changes

sphuber mentioned this pull request Jul 20, 2020

Bug in aiida.engine.daemon.execmanager.retrieve_files_from_list #3095

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all files from the pre-commit exclude list #4196

Remove all files from the pre-commit exclude list #4196

sphuber commented Jun 23, 2020 •

edited

Loading

codecov bot commented Jun 24, 2020 •

edited

Loading

sphuber commented Jun 24, 2020

chrisjsewell left a comment

sphuber commented Jun 24, 2020

ltalirz commented Jun 30, 2020 •

edited

Loading

sphuber commented Jun 30, 2020

ltalirz commented Jun 30, 2020 •

edited

Loading

chrisjsewell commented Jun 30, 2020

chrisjsewell Jul 1, 2020

sphuber Jul 1, 2020

ltalirz Jul 1, 2020

chrisjsewell Jul 1, 2020

chrisjsewell Jul 1, 2020

ltalirz Jul 1, 2020

sphuber Jul 1, 2020

chrisjsewell Jul 1, 2020

ltalirz Jul 11, 2020 •

edited

Loading

ltalirz Jul 11, 2020

sphuber Jul 12, 2020

sphuber Jul 12, 2020

ltalirz Jul 12, 2020

sphuber Jul 12, 2020

Remove all files from the pre-commit exclude list #4196

Remove all files from the pre-commit exclude list #4196

Conversation

sphuber commented Jun 23, 2020 • edited Loading

codecov bot commented Jun 24, 2020 • edited Loading

Codecov Report

sphuber commented Jun 24, 2020

chrisjsewell left a comment

Choose a reason for hiding this comment

sphuber commented Jun 24, 2020

ltalirz commented Jun 30, 2020 • edited Loading

sphuber commented Jun 30, 2020

ltalirz commented Jun 30, 2020 • edited Loading

chrisjsewell commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber commented Jun 23, 2020 •

edited

Loading

codecov bot commented Jun 24, 2020 •

edited

Loading

ltalirz commented Jun 30, 2020 •

edited

Loading

ltalirz commented Jun 30, 2020 •

edited

Loading

ltalirz Jul 11, 2020 •

edited

Loading