♻️ REFACTOR: Make `all` imports explicit (via pre-commit) #5061

chrisjsewell · 2021-08-10T22:39:56Z

This PR adds a pre-commit hook (in utils/make_all.py) which:

builds a tree of all non-init modules and their __all__ values
walks through __init__.py files and auto-generates the * imports of submodules and __all__ list for all (recursive) children
Returns non-zero if there are any unreadable __all__ and/or conflicting imports

This intends to address two related issues:

As discussed in Pre-commit: move yapf and mypy config to pyproject.toml #4996 (comment), the current * imports are incompatible with mypy, pylance and a number of other tools which utilise static analysis of the code.
The * imports make it extremely difficult to understand what is being exposed at each level of the API (Docs: clarify public API #4558), and can lead to unreported name clashes

Currently, this method works best when there is no actual code present in the init files, although I added # END AUTO-GENERATED to be able to leave aiida/manage/configuration/__init__.py, as the tests were failing when moving them to another module.

For a few other modules I moved the __init__.py content to a main.py

see if the tests pass 🤞

chrisjsewell · 2021-08-10T22:43:40Z

Hmm, workflows are failing on Ubuntu install; maybe GH is having some down time

chrisjsewell · 2021-08-10T23:19:02Z

utils/make_all.py

+
+if __name__ == '__main__':
+    _folder = Path(__file__).parent.parent.joinpath('aiida')
+    _skip = {


note also, here we specify children imports that should always be skipped

please document in a comment why these imports are being skipped (individually, if there are different reasons, or once if they are all skipped for the same reason - e.g. import clashes that should be fixed).

thanks!

for the 'orm': 'implementation' case - just to make sure I understand:
this is just a precaution, correct?
I.e. it would only take effect if we were to from implementation import *?

yep thats correct: I tried to keep it as close to the existing imports as possible for now, but obviously we can iterate on exactly what is exposed where

All the tests are passing, so hopefully that gives some sense of assurance that everythin is still exposed as before. and I had a quick check against: https://aiida.readthedocs.io/projects/aiida-core/en/latest/reference/api/public.html (although obviously that is missing a lot)

Ah, I didn't read the PR description carefully enough - I thought there might be some "priming" by the current imports, but what this PR does is to impose from ... import * for all submodules, correct?

I.e. there is no way to export a functionality only on aiida.x.y.z - it will always bubble up to aiida.x (unless it's in the skip list).
Is that correct?

In a way, it means that we are using __all__ (anywhere in the source tree) to define AiiDA's public API.

That is is ok, but I think we should document this and get input from some others to check they agree

Is that correct?

yep thats correct, and is basically what was already happening, just more manually 😬 (there was definitely places I think where people had just forgot to add certain imports)

there is no way to export a functionality only on aiida.x.y.z - it will always bubble up to aiida.x (unless it's in the skip list)

currently yeh, although obviously now the basic code is in place, I could always make it fancier lol, for more fine-grained control if necessary

codecov · 2021-08-10T23:34:06Z

Codecov Report

Merging #5061 (7e2dc6a) into develop (8e99581) will increase coverage by 0.13%.
The diff coverage is 94.53%.

@@             Coverage Diff             @@
##           develop    #5061      +/-   ##
===========================================
+ Coverage    80.25%   80.38%   +0.13%     
===========================================
  Files          515      529      +14     
  Lines        36753    36862     +109     
===========================================
+ Hits         29492    29627     +135     
+ Misses        7261     7235      -26

Flag	Coverage Δ
django	`74.88% <93.54%> (+0.15%)`	⬆️
sqlalchemy	`73.78% <93.10%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/common/__init__.py	`100.00% <ø> (ø)`
aiida/orm/__init__.py	`100.00% <ø> (ø)`
aiida/tools/data/array/kpoints/seekpath.py	`100.00% <ø> (ø)`
aiida/tools/data/structure.py	`91.97% <ø> (ø)`
aiida/manage/database/integrity/utils.py	`52.39% <52.39%> (ø)`
aiida/manage/tests/main.py	`88.54% <88.54%> (ø)`
aiida/tools/data/array/kpoints/main.py	`88.68% <88.68%> (ø)`
aiida/tools/importexport/dbimport/main.py	`94.12% <94.12%> (ø)`
aiida/tools/importexport/dbexport/main.py	`97.31% <97.31%> (ø)`
aiida/cmdline/params/options/main.py	`98.25% <98.25%> (ø)`
... and 105 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 8e99581...7e2dc6a. Read the comment docs.

chrisjsewell · 2021-08-10T23:56:55Z

all tests passing 🎉

ltalirz

thanks @chrisjsewell

As mentioned in the AiiDa meeting I think this will help us clean up the public API and be more mindful of what we expose going forward.

utils/make_all.py

ltalirz · 2021-08-11T09:37:14Z

utils/make_all.py

+
+if __name__ == '__main__':
+    _folder = Path(__file__).parent.parent.joinpath('aiida')
+    _skip = {


please document in a comment why these imports are being skipped (individually, if there are different reasons, or once if they are all skipped for the same reason - e.g. import clashes that should be fixed).

utils/make_all.py

Co-authored-by: Leopold Talirz <leopold.talirz@gmail.com>

ltalirz

thanks @chrisjsewell , for me this can proceed.
please wait for at least one more review e.g. from @sphuber or @greschd before merging.

I understand that the idea is to have a follow-up PR to update reference/api/public, both in terms of creating the list programmatically, and in documenting that the public API is simply everything people expose via __all__ anywhere inside the AiiDA codebase (with exceptions defined at ...)

chrisjsewell · 2021-08-11T11:13:41Z

Yeh so @sphuber @giovannipizzi @greschd, whoever lol, it would be good to get the review so I can move on to the next PRs (#5063, ##4996, ...)

sphuber

Thanks @chrisjsewell . Just some minor changes and questions and this would be good to go for now. There is one bigger question, but that would be for another PR. I think we are exposing too much right now, at least there are quite a few things that were never intended to be public facing API, and since we are now declaring that anything that is present in the init's of the second level modules, we will have to revise this. But as I said, I think we can address this in a separate PR, but this should be done before the 2.0 release. In that PR, we should also not forget to update the docs, which contains a hand-written page with public API resources. this should be removed.

sphuber · 2021-08-11T11:22:54Z

aiida/cmdline/utils/echo.py

@@ -18,7 +18,7 @@

 __all__ = (
    'echo', 'echo_info', 'echo_success', 'echo_warning', 'echo_error', 'echo_critical', 'echo_highlight',
-    'echo_dictionary'
+    'echo_dictionary', 'VALID_DICT_FORMATS_MAPPING'


Why does this need to be exported? I think this should remain internal.

Its used here: https://github.com/chrisjsewell/aiida_core/blob/e3b386196eb9d5b4be67b4bf979210269eca1900/aiida/cmdline/params/options/main.py#L581

But the fact that it is used internally by the module does not mean it should be in the __all__. Even if it is not there, it can still be imported without any issue. So I would really just revert this change as I don't think this should necessarily be exposed at the top level.

aiida/common/log.py

sphuber · 2021-08-11T11:27:16Z

aiida/cmdline/params/options/__init__.py

-)
-
-FORCE = OverridableOption('-f', '--force', is_flag=True, default=False, help='Do not ask for confirmation.')
+# yapf: disable


Since we are removing all other code from the init's and just have the __all__, why do we need to disable and reenable yapf? I know that yapf would change the one resource per line to just try and fit as much on one line, but why is this not desirable?

Well firstly, I feel it makes it less readable, but mainly I think it would make the pre-commit hooks clash, since each would keep changing it back and forth, and failing all the pre-commits

If we want to really keep it on one line, all you need to do to have yapf accept that is to also have a comma after the last entry . This is interpreted by yapf that we want to keep one entry per line..this would allow us to get rid of all the disable en able statements

In the spirit of this PR, I'm happy with the explicit nature of using # yapf: disable, rather than relying on some obscure interpretation of commas by yapf. So we'll agree to disagree on this one 😉

sphuber · 2021-08-11T11:30:14Z

aiida/orm/utils/load_funcs.py

@@ -0,0 +1,205 @@
+# -*- coding: utf-8 -*-


Would rename this file load_funcs.py to loaders.py.

Ha, so there is already a loaders.py in that module. Open to other suggestions though

Ah yeah of course, my bad. But perfect, that is where they should go. I would just put them at the top of that file.

aiida/common/log.py

chrisjsewell · 2021-08-11T11:47:16Z

I think we are exposing too much right now

Yep agree; in this PR I was just trying to keep ~parity with the existing installs, but we can certainly do that

chrisjsewell · 2021-08-11T11:47:43Z

cheers @sphuber, addressed your comments

chrisjsewell · 2021-08-11T11:49:15Z

I've removed #4558 from the linked PRs, to keep it open for the follow PR

ramirezfranciscof

Hey @chrisjsewell , amazing refactor! This makes the code feel much more organized, besides enabling the linting features you already mentioned. For me this would be good to merge once @sphuber remaining comments are addressed.

Perhaps there is only one thing I would point out: all this PR is almost purely the inclusion of the script, the moving around of content of the __init__ files to main files or the like (I understand you didn't modify any of this content), and the automated __all__ writting. There are however two modifications of content that I'll point out below. Are these strictly necessary? If they are just convenient, I would try to put them in a separate PR that we can review very quickly after this one. I hope it doesn't come out as too pedantic, but I just feel that mixing content modification on a PR that is heavily moving content may "hide" it if we need to revisit the changes at some point.

ramirezfranciscof · 2021-08-11T14:27:05Z

aiida/common/log.py

-            'handlers': ['console'],
-            'level': lambda: get_config_option('logging.aiida_loglevel'),
-            'propagate': False,
+def get_logging_config():


Are the changes in this file necessary for implementing the automation of __all__?

Yeh otherwise there were messy circular dependency errors.
Two key points here are (a) top-level imports should always flow from aiida.common to other modules (since its meant to be a common module) and (b) you should always try to avoid mutable global variables

ramirezfranciscof · 2021-08-11T14:28:35Z

setup.json

@@ -95,7 +95,7 @@
        ],
        "pre-commit": [
            "astroid<2.5",
-            "mypy==0.790",
+            "mypy==0.910",


Is this dependency bump necessary for implementing the automation of __all__?

Well, one of the primary purposes of this PR was to get the new version of mypy working, so I wanted to make sure that was actually the case

ramirezfranciscof · 2021-08-11T14:34:35Z

I think we are exposing too much right now

Yep agree; in this PR I was just trying to keep ~parity with the existing installs, but we can certainly do that

What would be the best way to do this later? We would need to use this kind of "exceptions" list in your automated code (this) or maybe if we are going to increase significantly the number of "unexposed" methods we need to find a cleaner way to do it (which is perhaps "positive" rather than "negative", i.e. we specify what we want to include instead of what we want to skip)?

Another question: does this now automatically clean up the autogenerated docs? Or if not, is there a way in which we could adapt it so that it does (so there is a specific section of the docs with only the exposed public API that is automatically generated)?

sphuber · 2021-08-11T15:05:45Z

What would be the best way to do this later? We would need to use this kind of "exceptions" list in your automated code (this) or maybe if we are going to increase significantly the number of "unexposed" methods we need to find a cleaner way to do it (which is perhaps "positive" rather than "negative", i.e. we specify what we want to include instead of what we want to skip)?

We simply remove from the __all__ of any module that should not be exposed at the top. If it is not in the __all__ it won't be bubbled up, but it can still be imported. So we just need to go through all the __all__'s and remove those resources that should not be considered public. That is all.

chrisjsewell · 2021-08-11T15:19:41Z

If it is not in the all it won't be bubbled u

Yeh so I would say we are indeed being "positive" rather than "negative", since you specify positively what you want exposed by adding it to its files __all__

chrisjsewell · 2021-08-11T15:32:34Z

does this now automatically clean up the autogenerated docs?

it doesn't yet but it can be easily added, see #4558 (comment)

chrisjsewell added 3 commits August 10, 2021 21:23

♻️ REFACTOR: Make __all__ explicit

351684c

improve

5a1ca68

pre-commit fixes

9d2e4d7

chrisjsewell requested review from sphuber and ltalirz August 10, 2021 22:41

chrisjsewell added 2 commits August 11, 2021 01:09

add load_entity to __all__

d5f4141

Update __init__.py

1462dff

chrisjsewell commented Aug 10, 2021

View reviewed changes

allow * skip

0e4d636

chrisjsewell changed the title ~~♻️ REFACTOR: Make __all__ imports explicit (add pre-commit)~~ ♻️ REFACTOR: Make __all__ imports explicit (via pre-commit) Aug 10, 2021

⬆️ UPDATE: mypy v0.910

1a55259

chrisjsewell linked an issue Aug 11, 2021 that may be closed by this pull request

Docs: clarify public API #4558

Closed

chrisjsewell mentioned this pull request Aug 11, 2021

Pre-commit: move yapf and mypy config to pyproject.toml #4996

Closed

Merge branch 'develop' into __all__

e23b6b4

chrisjsewell mentioned this pull request Aug 11, 2021

👌 IMPROVE: Add typing to QueryBuilder #5063

Merged

chrisjsewell requested a review from ramirezfranciscof August 11, 2021 09:05

Merge branch 'develop' into __all__

5b29b38

ltalirz suggested changes Aug 11, 2021

View reviewed changes

chrisjsewell commented Aug 11, 2021

View reviewed changes

utils/make_all.py Outdated Show resolved Hide resolved

Apply suggestions from code review

797b7f6

Co-authored-by: Leopold Talirz <leopold.talirz@gmail.com>

ltalirz self-requested a review August 11, 2021 11:01

ltalirz previously approved these changes Aug 11, 2021

View reviewed changes

chrisjsewell requested review from greschd and giovannipizzi August 11, 2021 11:07

Merge branch 'develop' into __all__

e3b3861

sphuber requested changes Aug 11, 2021

View reviewed changes

chrisjsewell commented Aug 11, 2021

View reviewed changes

aiida/common/log.py Outdated Show resolved Hide resolved

Update aiida/common/log.py

fce7f62

chrisjsewell dismissed ltalirz’s stale review via fce7f62 August 11, 2021 11:44

chrisjsewell requested a review from sphuber August 11, 2021 11:47

chrisjsewell removed a link to an issue Aug 11, 2021

Docs: clarify public API #4558

Closed

ramirezfranciscof previously approved these changes Aug 11, 2021

View reviewed changes

chrisjsewell added 2 commits August 11, 2021 16:39

Move loaders

70868bf

remove VALID_DICT_FORMATS_MAPPING

7e2dc6a

chrisjsewell dismissed ramirezfranciscof’s stale review via 7e2dc6a August 11, 2021 14:40

chrisjsewell merged commit ead3f39 into aiidateam:develop Aug 11, 2021

chrisjsewell deleted the __all__ branch August 11, 2021 15:10

chrisjsewell mentioned this pull request Aug 11, 2021

Docs: clarify public API #4558

Closed

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

♻️ REFACTOR: Make __all__ imports explicit (via pre-commit) #5061

♻️ REFACTOR: Make __all__ imports explicit (via pre-commit) #5061

Conversation

chrisjsewell commented Aug 10, 2021 • edited Loading

chrisjsewell commented Aug 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz Aug 11, 2021 • 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

ltalirz Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

chrisjsewell Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2021 • edited Loading

Codecov Report

chrisjsewell commented Aug 10, 2021

ltalirz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltalirz left a comment • edited Loading

Choose a reason for hiding this comment

chrisjsewell commented Aug 11, 2021

sphuber left a 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

chrisjsewell Aug 11, 2021 • 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

chrisjsewell Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell commented Aug 11, 2021

chrisjsewell commented Aug 11, 2021

chrisjsewell commented Aug 11, 2021

ramirezfranciscof left a 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

ramirezfranciscof commented Aug 11, 2021

sphuber commented Aug 11, 2021

chrisjsewell commented Aug 11, 2021

chrisjsewell commented Aug 11, 2021

♻️ REFACTOR: Make `all` imports explicit (via pre-commit) #5061

♻️ REFACTOR: Make `all` imports explicit (via pre-commit) #5061

chrisjsewell commented Aug 10, 2021 •

edited

Loading

ltalirz Aug 11, 2021 •

edited

Loading

ltalirz Aug 11, 2021 •

edited

Loading

chrisjsewell Aug 11, 2021 •

edited

Loading

codecov bot commented Aug 10, 2021 •

edited

Loading

ltalirz left a comment •

edited

Loading

chrisjsewell Aug 11, 2021 •

edited

Loading

chrisjsewell Aug 11, 2021 •

edited

Loading