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

Isolate qibo-core from qibo #33

Merged
merged 8 commits into from
Jun 20, 2024
Merged

Isolate qibo-core from qibo #33

merged 8 commits into from
Jun 20, 2024

Conversation

alecandido
Copy link
Member

During the quick import process of some of the Qibo's modules, in order to make #26 possible, we got a definitely unwanted dependency on qibo itself.

One way to sort it out is to migrate even more code from qibo to qibo-core, in order to make a closure of the internals.
The other way is to remove code that is currently unused (corresponding to features that are yet unsupported by qibo-core, or will be replaced, or that will never be supported as they remain part of only qibo, as high-level features).

In this PR, the goal is to apply a combination of both solutions, and to fully drop the dependency on qibo, to make qibo-core working in isolation.

@alecandido
Copy link
Member Author

In 5e3e821 I migrated the measurements module. Why it didn't come with additional internal imports (from qibo), it added a dependency on Sympy.

Since this dependency corresponds to a feature already scheduled for replacement with a simpler one (cf. #32), we should just drop the code making use of Sympy, and consequently drop the import.

@alecandido
Copy link
Member Author

@stavros11 this should also be ready for review (and possibly merge).

I ended up migrating both measurements.py and backends.py (i.e. qibo/backends/__init__.py).

I acted a bit blindly, without thinking too much, and there is a lot of unused code. However, we know this code is not here to stay, not even the NumpyBackend.
I'd suggest to keep playing with the code in here for another short while. As soon as most of the features listed in the issues will be implemented (or the more remarkable ones), I'd propose to scratch all the Python code in qibo_core_py/qibo_core, and start working with a PR in qibo itself, where we start importing from qibo_core, and we'll check how many qibo tests will fail.

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alecandido, looks good to me. The comments are just for getting rid some of the things we will probably never need, however given that this structure is temporary anyway, I wouldn't bother much.

I ended up migrating both measurements.py and backends.py (i.e. qibo/backends/__init__.py).

As written in one of the comments, I think we can relatively easily get rid of backends.py, at least temporarily.

As soon as most of the features listed in the issues will be implemented (or the more remarkable ones), I'd propose to scratch all the Python code in qibo_core_py/qibo_core, and start working with a PR in qibo itself, where we start importing from qibo_core, and we'll check how many qibo tests will fail.

I would also like to do that as soon as possible, because there is a lot of testing already implemented in qibo which could tell us if we are moving in the correct direction without the additional effort of rewriting many tests (which we should also do on qibo-core, but maybe not all at once).

We just need to identify which issues are the "remarkable" issues. We will need to implement everything to pass all tests, however I'd say that the migration makes sense after #7, #8, #22, #29 and #30. These should also be sufficient for the majority of qibo applications. The rest (callbacks, collapse, channels, etc.) can be thought as additional enhancements.

Note that we will also need to make qibo-core public for this, if we want easy usage in qibo CI.


self.backend = _check_backend(backend)
self.backend = backends._check_backend(backend)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we just hardcode the numpy backend here, so basically

Suggested change
self.backend = backends._check_backend(backend)
from .numpy import NumpyBackend
self.backend = NumpyBackend()

and the same in the other two instances where .backends is used (where numpy is already hardcoded), we shouldn't need moving the backends module here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but qibo-core aims to be used inside qibo. I decided to drop Sympy because of the extra dependency, but here I will try to stay closer to the Qibo code, to simplify the transition later on.

Let's review this point in the subsequent developments.

@@ -30,15 +33,16 @@ class QuantumState:
"""

def __init__(self, state, backend=None):
from qibo.backends import _check_backend
from . import backends
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier when I was looking at this PR and wanted to replace qibo.backends with just the NumpyBackend, I noticed that there will be a circular import issue. At some point, I would like to resolve this by trying to remove the backend attribute from the QuantumState and all other result objects. Should we open an issue about this (most likely in qibo)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely in Qibo.

Though I fear we'll need to store some backend identifier, because there will be people receiving this object and attempting to manipulate it, that will need to know which backend to use (i.e. @MatteoRobbiati in qiboml).

However, the ideal solution would be to decouple the state manipulation library from the backend (because we'll need one library per platform, i.e. CPU and GPU - more if it's impossible to cast arrays from one library to another one, that for the time being it should not be the case, just using DLPack and similar compatibility protocols), and just have different results types, each one referred to the suitable library, like:

class Result:
    ...

class CPUResult(Result):
    pass

class GPUResult(Result):
    pass

or convert the subclasses into an attribute tag (I would keep the state manipulation library separate from the result object anyhow, not exposing them as methods).

qibo-core-py/qibo_core/measurements.py Outdated Show resolved Hide resolved
Base automatically changed from queue to main June 18, 2024 06:54
@alecandido
Copy link
Member Author

We just need to identify which issues are the "remarkable" issues. We will need to implement everything to pass all tests, however I'd say that the migration makes sense after #7, #8, #22, #29 and #30. These should also be sufficient for the majority of qibo applications. The rest (callbacks, collapse, channels, etc.) can be thought as additional enhancements.

You could collect these issues in a Milestone.

Note that we will also need to make qibo-core public for this, if we want easy usage in qibo CI.

This will happen even before (i.e. I'm planning to merge this and make it public).

@alecandido
Copy link
Member Author

Thanks @stavros11 for the review, I'm merging this and start working on the workflows.

@alecandido alecandido merged commit 0213f96 into main Jun 20, 2024
@alecandido alecandido deleted the isolate-from-qibo branch June 20, 2024 09:15
@stavros11
Copy link
Member

You could collect these issues in a Milestone.

Done in https://github.com/qiboteam/qibo-core/milestone/1. Feel free to modify this.
I included only issues that are relevant for the most basic qibo applications (circuit simulation), excluding the more advanced features. I am pretty sure about the list, except maybe #20 which may not be needed in the first iteration, since we already have the NumpyMatrices working, even if not in the most ideal way.

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.

2 participants