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

chore(deps): bump pin on numpy requirement #1524

Closed
wants to merge 3 commits into from

Conversation

robertodr
Copy link

Summary

NumPy is only needed to run tests and examples. I've moved it to requirements-dev.txt as otherwise downstream packages cannot update to NumPy >=2.0.

Details and comments

@CLAassistant
Copy link

CLAassistant commented Oct 22, 2024

CLA assistant check
All committers have signed the CLA.

@Tansito Tansito requested a review from a team October 22, 2024 12:16
Copy link
Member

@Tansito Tansito left a comment

Choose a reason for hiding this comment

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

First of all thank you so much for your contribution @robertodr

client is our library used in our ray-node image. Taking this into account numpy is a library very used in our entire stack so I don't think we can remove it so easily because it could be needed to run other functions that count with it.

My proposal would be to follow a similar requirement as Qiskit:

numpy>=1.17,<3

So v2 would be supported.

@Tansito Tansito requested a review from paaragon October 22, 2024 12:21
@robertodr
Copy link
Author

@Tansito I would be happy with lifting the version pinning as you suggest. However, keeping a package you don't directly depend on in your install_requires, just because other dependents might need it, is, as far as I know, bad practice. The dependents should explicitly list that package in their install_requires.

@Tansito
Copy link
Member

Tansito commented Oct 22, 2024

Well, our use case it's a bit special due to the usage that we are doing with our docker image. But thinking a bit more about it I think you are right here and it's better to move it to our -dev dependencies. Could you update your change with the same restriction as qiskit as we were discussing above?

Apart from that I think everything is fine 👍

@robertodr robertodr requested a review from Tansito October 23, 2024 06:51
@Tansito
Copy link
Member

Tansito commented Oct 23, 2024

It seems there is an error in one of the tests @robertodr , I will take a look into it and let you know

@Tansito
Copy link
Member

Tansito commented Oct 23, 2024

@robertodr from the tests it seems numpy is a required dependency, without it serverles is not able to run the functions. Could you apply the restriction in the requirements.txt?

We will take a look in the meantime what dependency requires to have numpy declared there, it's not clear. Thank you!

@robertodr robertodr changed the title chore: only require numpy in development chore(deps): bump pin on numpy requirement Oct 25, 2024
@Tansito
Copy link
Member

Tansito commented Oct 28, 2024

sorry @robertodr it seems we continue with some incompatibilities, tomorrow I will take a look and let you know 🙏

@Tansito
Copy link
Member

Tansito commented Oct 30, 2024

@robertodr I could finally identify the problem and we will take a look as soon as we can. I created this issue to keep you updated about the advances: #1533

I will close this PR in the meantime and open a new one as soon as we have it fix it!

@Tansito Tansito closed this Oct 30, 2024
@robertodr robertodr deleted the patch-1 branch October 30, 2024 12:24
@robertodr
Copy link
Author

Ok thanks. Will subscribe to that issue

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.

4 participants