search

gnocchixyz / gnocchi

Timeseries database
Apache License 2.0
299 stars 85 forks source link

Enable the upgrade to numpy 1.24 #1279

Closed rafaelweingartner closed 1 year ago

rafaelweingartner commented 1 year ago

@jd and @chungg can you take a look into this one. It is an important update. Otherwise, the pipeline starts to break.

rafaelweingartner commented 1 year ago

@tobias-urdin can you take a look into this one as well?

rafaelweingartner commented 1 year ago

@tobias-urdin I applied your suggestions. Can you check the PR again?

rafaelweingartner commented 1 year ago

@tobias-urdin if I remove the downgrade of numpy the tests do not pass, as we install in the upgrade tests the Gnocchi version from stable/4.4, which is not ready for numpy 1.24, which is the one installed by default.

tobias-urdin commented 1 year ago

You should propose a commit that pins numpy, commit(s) that fixes support for newer numpy, create a commit that reverts commit 1.

rafaelweingartner commented 1 year ago

You should propose a commit that pins numpy, commit(s) that fixes support for newer numpy, create a commit that reverts commit 1.

I think that I did not get it. What is your proposal?

tobias-urdin commented 1 year ago

Yes, like that, I should have explained myself better. You can do it as commits doesn't have to be a separate PR but would be preferable so that we can verify the CI gets green between changes.

tobias-urdin commented 1 year ago

fixed in https://github.com/gnocchixyz/gnocchi/pull/1281

rafaelweingartner commented 1 year ago

@tobias-urdin thanks for merging the proposal. I would execute the process we agreed on. However, I had no spare time this week. You seem to have done it differently from what we discussed though. I do not understand how the upgrade tests passed with your PR though...

One remark regarding the process, from an opensource perspective, I would have preferred to discussed the alternative if you wanted to move on with this quickly. Otherwise, it seems that people just take over other people proposal, which might not sound welcoming to contributors.

tobias-urdin commented 1 year ago

Hello 👋 had to pin numpy in stable/4.4 [1] since upgrade jobs on master install old version from stable/4.4 branch and then new version from master. I then simply backported the change on master to stable/4.4 to unpin numpy again on stable/4.4

[1] https://github.com/gnocchixyz/gnocchi/pull/1282

rafaelweingartner commented 1 year ago

I see, so you implemented what we discussed. Thanks for that.