Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check derivative imports #949

Closed
alecandido opened this issue Aug 5, 2024 · 1 comment · Fixed by #1030
Closed

Check derivative imports #949

alecandido opened this issue Aug 5, 2024 · 1 comment · Fixed by #1030
Assignees
Milestone

Comments

@alecandido
Copy link
Member

alecandido commented Aug 5, 2024

We should avoid importing from subpackages within the subpackage itself, since it is likely to generate circular imports (if not immediately, later on).

Consequently, importing directly from qibolab (without further qualifications) should never appear within the whole package.

Moreover, if #790 is not fully applied, we should check that no import is making use of accidental re-exports (i.e. places where an object is actually imported, but to be used, not as an intentional export - as it should happen in the various __init__.py only).


I'll try to establish a few rules (more guidelines) to how importing things

  1. never import from a place which is not supposed to export
    • the export should be contained in __all__, but we won't be strict on that, not immediately, so the "export places" are either those where the objects are initially defined, or on-purpose re-exports from the various __init__.py
    • example: if an object is defined in a.b.c.d.e, you should import it from there, or from: a.b.c.d, a.b.c, a.b
  2. never import from any of your own __init__.py
    • __init__.py is meant for the export, the first thing encountered by anyone looking at the (sub)package from the outside, and it could contain re-exports -> high chance of circular imports
    • example: if you are a.b.c, never import from a.b nor a -> no one in Qibolab should import from qibolab itself
  3. if importing from another subpackage, import at the top-most level possible
    • at least the internals can be changed at will, the compatibility towards the outside has only to be honored in a single place
    • example: if you are a.b, and import from a.c.d.e, then import from a.c, if it exports it
  4. * imports are recommended for re-exporting the content of the modules in the __init__.py above, but they should be avoided for internal usage
  5. since it seems that the majority of tools understand the __all__ += submodule.__all__ idiom, but not all the list methods (like .extend()), and not even list comprehension, the redundant syntax above is suggested, namely:
    1. * export the content of all the submodules
    2. import all the submodules as module objects, to access their .__all__ attributes
    3. extend the parent __all__ with += statements, one at a time
    • it is definitely redundant, since modules are listed three times, but this is to ensure the widest compatibility possible with external static tools (i.e. not executing the code in a Python interpreter, e.g. linters as pylint & ruff, type checkers as mypy & pyright, and others as pycln)
@alecandido alecandido added this to the Qibolab 0.2.0 milestone Aug 5, 2024
@alecandido alecandido self-assigned this Aug 5, 2024
This was referenced Sep 2, 2024
@alecandido alecandido linked a pull request Sep 6, 2024 that will close this issue
1 task
@alecandido
Copy link
Member Author

@stavros11 I was thinking to keep the guide above in some file in the repo. We could eventually export to general Qibo developer docs, but for the time being this will only be used in Qibolab, so I'd keep it here (for as long as we do not decide we like this public API definition, and start exporting it to the other packages).

I was thinking to CONTRIBUTING.md, but we can even add a developers' section to the Qibolab docs (additional to the link to Qibo's)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant