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

Drop explicit numpy dep #64

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Drop explicit numpy dep #64

wants to merge 2 commits into from

Conversation

BrunoLiegiBastonLiegi
Copy link
Contributor

As per title, this causes conflicts with qibo's numpy dependency, and since qibo is already a qiboml dependency there should not be the need to explicit depend on numpy

@alecandido
Copy link
Member

There are at least a few places where NumPy is directly used

https://github.com/search?q=repo%3Aqiboteam%2Fqiboml+%22import+numpy%22&type=code

E.g.

import numpy.typing as npt



import numpy as np

https://github.com/qiboteam/qiboml/blob/96f7c471286192ef0e43295e811878c57d3acafe/src/qiboml/models/ansatze.py

Either you drop them all, or you should keep the dependency explicit. If you do not want to impose conflicting bounds, you could go for something like numpy = ">=1.26.4", and accept even breaking releases.

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

Yeah but numpy should always be available whether or not qiboml explicitely depends on it, since it is a qibo dependency which is a mandatory dependency for qiboml, right?

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.85%. Comparing base (5cb04ad) to head (ef35fde).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #64   +/-   ##
=======================================
  Coverage   87.85%   87.85%           
=======================================
  Files          11       11           
  Lines         494      494           
=======================================
  Hits          434      434           
  Misses         60       60           
Flag Coverage Δ
unittests 87.85% <ø> (ø)

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

@alecandido
Copy link
Member

Yeah but numpy should always be available whether or not qiboml explicitely depends on it, since it is a qibo dependency which is a mandatory dependency for qiboml, right?

This is a quite wicked way to declare dependencies...

If you use something directly, you should declare as a dependency. And not relying on transitive dependencies.
The point is that Qibo is not exposing NumPy as part of its public interface, so (in principle) it may drop it at any time. Which dependency a package is using internally is not guaranteed to the users, as an implementation detail (despite being declared in manifests for installation purposes).

Moreover, if you have restrictions on some dependency versions, you should specify them, since they can further restrict the domain of possible solutions.

Here the problem is that you expect them to be plainly conflicting. In this case, if you wish to prevent the conflict, you should accept also the portion of the version range you expect your other dependency to impose, in order to make a solution possible. In practice, it is what you will receive even when you do not specify anything (since the version of the transitive dependency, i.e. NumPy in this case, will be imposed by the other package).

So, to summarize, better to allow both NumPy 1 and 2 explicitly, rather than saying nothing, and then receiving NumPy 2, even if you want to preserve compatibility with NumPy 1 as well.

If you do not care about NumPy 1, you can drop everywhere, including here as well. This is just a trade-off: keeping both may be annoying, because you should limit yourself to use the part of the interface which is unchanged. You may try, and in case it gets complicated, just choose (or choose directly, w/o without for things getting complicated).

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