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

Mark private functionalities with an underscore to separate better from public API #6636

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

agoscinski
Copy link
Contributor

@agoscinski agoscinski commented Nov 25, 2024

The AiiDA public API has been defined in the documentation as every import that can be done from a secondary level. This is however not transparent to users and plugin developers as it is hidden in the doc resulting in a lot of misuse of private functionalities. The widespread use of these private functionalities blocks improvements in the code as we are more hesitant in changing functionalities for wrong backwards compatibility.

This PR marks clearly the internal functionalities with an underscore so a misuse only can happen intentionally. It only marks the third level of import with an underscore since any subsequent imports are automatically marked as internal. This gives us the opportunity to improve the public API as we can implement replacements for functionalities in the private API that are widely used.

This means the public API example changes to

from aiida import load_profile  # OK, top-level import
from aiida.orm import QueryBuilder  # OK, second-level import
from aiida.tools._importexport import Archive # NOT PUBLIC API

EDIT: Need to adapt imports in tests, and docs

The AiiDA public API has been defined in the documentation as every import that
can be done from a secondary level. This is however not transparent to users
and plugin developers as it is hidden in the doc resulting in a lot of misuse
of private functionalities. The widespread use of these private functionalities
blocks improvements in the code as we are more hesitant in changing
functionalities for wrong backwards compatibility.

This PR marks clearly the internal functioanlities with an underscore so a
misuse only can happen intentionally. This gives us the opportunity to improve
the public API as we can implement replacements for functionalities in the
private API that are widely used.
@agoscinski agoscinski changed the title Mark private API with an underscore Mark private functionalities with an underscore to separate better from public API Nov 25, 2024
@unkcpz
Copy link
Member

unkcpz commented Dec 10, 2024

I'd say we push on this and at the meanwhile improve the documentation?
I think we can really benefit from this change for deprecating things more confident. One example is #6656, I don't think we sometime overthink about backward compatibility since there is no clear boundary for what should be support for backward compatibility.

One thing to keep mind was such change will break some plugins that heavily use unexposed APIs, there may not too much for aiida-quantumespresso but for sure there are quite some for aiida-workgraph. I think we can still push for this PR and then goes to plugins and contribute to compatible with this change.

  • aiida-quantumespresso
  • aiida-workgraph
  • aiida-vasp
  • (please add more of what we should go to and add support proactively)

Want to hear your inputs @giovannipizzi @sphuber @superstar54 @mbercx

@superstar54
Copy link
Member

Thanks for the work.
How do we define which are public and which are private for some functionalities in a deep folder level? for example, aiida.engine.process.control, which I think we want to expose it as a public API.

@sphuber
Copy link
Contributor

sphuber commented Dec 10, 2024

I am personally not a fan of this approach as it doesn't really help with the problem. There may be plenty of users that are not at all familiar with the implicit meaning of a file starting with an underscore. Moreover, as @superstar54 already hinted, there will be resources that are deeper than two levels and should still be public. How will it be made clear which of those can be imported?

I think the most common approach for libraries is to simply expose everything that is public API directly at the top-level module. So in aiida/__init__.py you would add an __all__ and add all classes, functions etc to it that should be public. This makes it very clear to users what can be freely used and to developers what should not be broken in terms of interface/behavior.

The only potential problem I see is that with a package as large as aiida-core, there may be name clashes when putting all the public API in a single namespace. But not sure if we would have concrete examples of that.

If we are going to break things with this change, I think it would be advisable to do it properly in one go and really think hard what should be part of the public API.

@unkcpz
Copy link
Member

unkcpz commented Dec 11, 2024

How do we define which are public and which are private for some functionalities in a deep folder level? for example, aiida.engine.process.control

I think it depends, when the very deeply defined functions are used, it means the user is looking in the src code instead of the documentation to do some magics. It usually manifest two things and nudge us to take some operations:

  1. if it is clearly an handy API, we expose it.
  2. it is something that we need absorbing into aiida-core or it requires more basic support to make the magic easy to happen. (what comes to my mind were, aiida-workgraph, process monitor and async ssh transport).

I think the most common approach for libraries is to simply expose everything that is public API directly at the top-level module. So in aiida/init.py you would add an all and add all classes, functions etc to it that should be public.

I agree, especially after the discussion during the coding week with @agoscinski and @khsrali, I also read https://benhoyt.com/writings/python-api-design/. One of the take home message there is "Design your library to be used as import lib ... lib.Thing() rather than from lib import LibThing ... LibThing().". For aiida case is using from aiida import orm and then orm.Int is not so good since the "orm" is very domain specific and new users will confused with the terminologies.
But we made the decision to put the underscore mostly to keep the decision made from https://aiida.readthedocs.io/projects/aiida-core/en/stable/reference/api/public.html. I am not sure how that decision was made, but if we are going to break the APIs, we can more aggressive and make it long term proof.

@khsrali
Copy link
Contributor

khsrali commented Dec 11, 2024

@unkcpz take some sleep bro, it's 2:47 am 😅

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 this pull request may close these issues.

5 participants