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

Public API #790

Closed
alecandido opened this issue Jan 29, 2024 · 5 comments · Fixed by #1030
Closed

Public API #790

alecandido opened this issue Jan 29, 2024 · 5 comments · Fixed by #1030
Assignees
Milestone

Comments

@alecandido
Copy link
Member

alecandido commented Jan 29, 2024

Since Qibolab is now being used, and we transitioned to proper releases, in order to make the semver conversion even more meaningful we should define what is part of the public API of Qibolab.

The convention in Python is that everything that is accessible passing through a chain of non-underscore-led identifiers is public, i.e.:

# public
ciao
ciao.come
ciao.come.va

# private
_ciao
ciao._come
ciao.come._va
# and so also:
ciao._come.va
# as parte of `ciao._come`

However, if you use ciao._come.va inside ciao, importing directly the identifier, i.e.

# src/ciao.py
from ._come import va

this becomes accessible as ciao.va, and thus public again.

In general, the idea is to keep all the definitions internal, and exports only the public API through suitable __init__.py and modules imported by that, as a restricted hierarchy within the package, just made for re-exports.
Often, a _core subpackage, or something like that, is used to separate what is internal from the public API.

To simplify the management of the public API, there are different tools available in Python's module system:

  • __all__: is an attribute that specifies what is imported from this module when doing from mymodule import * - usually I would discourage import *, but it's convenient for re-exports
  • __getattr__ is customizable at module-level, and it could be used to provide dynamic loading (that for us could be useful to export the drivers)
  • similarly to the previous, even __dir__ is customizable in the same way

NumPy is quite a full-fledged example of how all of these are used to define the public API. It is also over-complicate for us, since they have decades of legacy code and users to support, but it's interesting to navigate their __init__.py structure.

This will help to identify which changes are actually breaking, and which are not.

(In principle, whatever is not part of the public API should also be kept internal, and not documented in Sphinx - I guess this should work automatically, ensuring meaningful __dir__ values - and it should not be an incentive to avoid docstrings, even for internal elements)

@alecandido
Copy link
Member Author

A first effort should be made in 0.2, to declare which is the interface we're trying to avoid breaking.

@alecandido
Copy link
Member Author

alecandido commented Aug 17, 2024

Let's try to keep a list of what is going to be included in and excluded from the interface in 0.2.

In
  • Pulse and all PulseLike objects
  • Envelope and its subclasses
  • Sweeper
  • PulseSequence
  • Platform
    • we should trim down the interface to the minimal subset
  • create_platform
  • Qubit
  • main instrument classes
    • only those strictly needed for platforms' creation
  • Channel and its subclasses
    • required for platform creation
  • MetaBackend and QibolabBackend
  • __version__
  • ExecutionParameters
    • this could be removed in 0.3, but let's keep it for the time being
Out
  • Compiler
    • custom compilers will be excluded from 0.2.0 (but they could be allowed in a subsequent 0.2.x)
  • Instrument
  • qibolab.pulses.plot, whole content
  • qibolab.serialize, whole content
  • qibolab.native, whole content
    • natives classes are should only be needed internally for deserialization
  • qibolab.unrolling, whole content
  • qibolab.result
  • Parameters (and qibolab.parameters in general)
    • they may not be needed at all during platform creation (and then never needed)
  • Config and its subclasses (qibolab.components.configs)
    • possibly exposed in a subsequent released, for advancedusers
  • execute_qasm
    • still available internally, but no guarantee is made on its stability
  • ChannelId
    • just a str in the interface - let's try like this

@stavros11
Copy link
Member

stavros11 commented Aug 21, 2024

I agree with the summary and in/out.

  • create_dummy

Shouldn't this be create_platform (which also includes the dummy)?

Regarding the "doubts":

  • ChannelId

    • I wonder whether to keep this internal, and only expose strings (somehow)
  • Qubit

    • depending on how the channels are accessed, maybe they are not needed in the interface

Will depend on #970. I am not sure if in the end you will give a try to remove Qubit completely (?)

  • Parameters (and qibolab.parameters in general)

    • they may not be needed at all during platform creation (and then never needed)

So far it seems to me that they are not needed for platform creation. For other usage, maybe we just need to pay the price of exposing only the things we need through Platform properties, like we did for native gates. So I'd move to "out".

  • Config and its subclasses (qibolab.components.configs)

    • essentially depend on Parameters, the decision will be the same

These may be needed for advanced users who want to use ExecutionParameters.updates. In principle the mechanism for this is in place, however this is not tested (as many other things in 0.2), not documented and I haven't even used it myself, so I am not sure if I would advertise it as part of the API immediately. I'd put in the same category as the Compiler.

  • execute_qasm

    • do we still need it? maybe it would make sense to move somewhere else, if needed there (qibo-client?)

Personally I found this useless even in 0.1 and my opinion remains the same. However, I would guess that the reasons we implemented it also remain the same, so most likely we need it.

  • ExecutionParameters

    • the class is useful internally, but maybe we could simplify it in the interface

We certainly need it, because there is no other to specify even a number of shots (different than default) when executing something. That being said, I am not a huge fan of it, so if you have suggestions to simplify they are welcome.

  • Envelope and its subclasses?

I think it is needed if someone wants to use the pulse API directly, not through the native gates as we typically do. If Pulse is exposed, this will probably need to be exposed as well. If we only use natives, we don't even need the Pulse. Only PulseSequence and maybe Delay.

@alecandido
Copy link
Member Author

Shouldn't this be create_platform (which also includes the dummy)?

Oh, yes, definitely.

Will depend on #970. I am not sure if in the end you will give a try to remove Qubit completely (?)

I will, let's see.

So far it seems to me that they are not needed for platform creation. For other usage, maybe we just need to pay the price of exposing only the things we need through Platform properties, like we did for native gates. So I'd move to "out".

Agreed.

These may be needed for advanced users who want to use ExecutionParameters.updates. In principle the mechanism for this is in place, however this is not tested (as many other things in 0.2), not documented and I haven't even used it myself, so I am not sure if I would advertise it as part of the API immediately. I'd put in the same category as the Compiler.

Ok, let's keep it temporarily "out" then, and provide this as a further feature at a later stage.

Personally I found this useless even in 0.1 and my opinion remains the same. However, I would guess that the reasons we implemented it also remain the same, so most likely we need it.

The problem is that now we're committing to stability, so we either remove it now or next is in 0.3 (and we have to maintain until then).

We certainly need it, because there is no other to specify even a number of shots (different than default) when executing something. That being said, I am not a huge fan of it, so if you have suggestions to simplify they are welcome.

Yeah, that's the thing I was unsure about. An option is to keep a **kwargs argument in execute() and pass them to ExecutionParameters. However, just doing this would make the nshots support undocumented, or we should duplicate the documentation for ExecutionParameters.
The other option is to turn it in a TypedDict[...], and use it exactly to hint the execute()'s **kwargs, and nothing else. But it's not that different (maybe just slightly more convenient to write during calls).

I think it is needed if someone wants to use the pulse API directly, not through the native gates as we typically do. If Pulse is exposed, this will probably need to be exposed as well. If we only use natives, we don't even need the Pulse. Only PulseSequence and maybe Delay.

Ok, but this to me sounds a good argument to expose them.

@alecandido alecandido mentioned this issue Sep 6, 2024
1 task
@alecandido alecandido linked a pull request Sep 6, 2024 that will close this issue
1 task
@alecandido
Copy link
Member Author

For the time being, I will apply a simplified strategy for optional dependencies, i.e. concerning drivers.

However, we may want to reassess the issue later on, as it is not that trivial, and I'm not sure which is the best way to handle them since I'd like:

  1. to declare them as part of the public API, but
  2. conditional to the presence of the required dependencies, possibly
  3. avoiding breaking static analysis

Pandas as a whole function (and module, https://github.com/pandas-dev/pandas/blob/main/pandas/compat/_optional.py) dedicated to the purpose, and even the TYPE_CHECKING and similar variables could be used as gates.

But since the problem is non-trivial, I would adopt a more poor-man solution, and then reconsider.

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.

2 participants