Enable the upgrade to numpy 1.24 #1279
Conversation
rafaelweingartner
force-pushed
the
fix_for_numpy.1.24
branch
11 times, most recently
from
b08e790 to
ca30e14
Compare
rafaelweingartner
force-pushed
the
fix_for_numpy.1.24
branch
from
ca30e14 to
134e10b
Compare
rafaelweingartner
force-pushed
the
fix_for_numpy.1.24
branch
from
134e10b to
0fa6f62
Compare
|
@tobias-urdin can you take a look into this one as well? |
run-upgrade-tests.sh
Outdated
| # https://github.com/jd/pifpaf/commit/fb376a83a47d678952672a7f5d36a02101135fb2, | ||
| # but it has never been released. Therefore, we need to install it here from | ||
| # master/main branch in the upstream repository | ||
| pip install install git+https://github.com/jd/pifpaf.git@51f74a3d8743a7ac33259413df7efc30df993460 |
There was a problem hiding this comment.
Please set this in tox.ini instead, you can see that I have some old pins for pifpaf there you can change those as the one I pin there is probably merged already.
There was a problem hiding this comment.
I fixed in tox then.
run-upgrade-tests.sh
Outdated
| @@ -51,15 +62,15 @@ inject_data() { | |||
|
|
|||
| { | |||
| measures_sep="" | |||
| MEASURES=$(python -c 'import datetime, random, json; now = datetime.datetime.utcnow(); print(json.dumps([{"timestamp": (now - datetime.timedelta(seconds=i)).isoformat(), "value": random.uniform(-100000, 100000)} for i in range(0, 288000, 10)]))') | |||
| MEASURES=$(python -c 'import datetime, random, json; now = datetime.datetime.utcnow(); print(json.dumps([{"timestamp": (now + datetime.timedelta(seconds=i)).isoformat(), "value": random.uniform(-100000, 100000)} for i in range(0, 288000, 10)]))') | |||
There was a problem hiding this comment.
Not sure why this is changed?
There was a problem hiding this comment.
It was changed during my tests, and I forgot to revert it.
| # https://github.com/gnocchixyz/gnocchi/pull/1279, which is the PR that | ||
| # introduces the support of numpy >= 1.24. After we merge it, we can remove | ||
| # this downgrade here. | ||
| pip install "numpy<1.24" |
There was a problem hiding this comment.
Pin this separately in setup.cfg and then propose a commit following the changes required that unpins it.
There was a problem hiding this comment.
I am not sure I follow your idea of creating a new commit. It will not work if I do that in this PR.
There was a problem hiding this comment.
You can remove this entirely as it's not needed right?
setup.cfg
Outdated
| @@ -26,7 +26,7 @@ packages = | |||
| include_package_data = true | |||
|
|
|||
| install_requires = | |||
| numpy>=1.9.0 | |||
| numpy>=1.19.5 | |||
There was a problem hiding this comment.
Is this the new lowest required verison for numpy.integer?
It's very high which means we'd get issues with some distributions that ship an older version, for example CentOS Stream 8 has 1.14.3 and CentOS Stream 9 has 1.20.1, I haven't checked Ubuntu.
There was a problem hiding this comment.
You are right, we do not need to change the lowest. The Integer member has always been there. However, the int is the one that was removed in the 1.24.0 version. I will amend my patch
mergify
bot
dismissed
tobias-urdin’s stale review
Pull request has been modified.
rafaelweingartner
force-pushed
the
fix_for_numpy.1.24
branch
2 times, most recently
from
03ebd33 to
0fa6f62
Compare
rafaelweingartner
changed the title
|
@tobias-urdin I applied your suggestions. Can you check the PR again? |
| # https://github.com/gnocchixyz/gnocchi/pull/1279, which is the PR that | ||
| # introduces the support of numpy >= 1.24. After we merge it, we can remove | ||
| # this downgrade here. | ||
| pip install "numpy<1.24" |
There was a problem hiding this comment.
You can remove this entirely as it's not needed right?
mergify
bot
dismissed
tobias-urdin’s stale review
Pull request has been modified.
|
@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. |
rafaelweingartner
force-pushed
the
fix_for_numpy.1.24
branch
from
890993b to
e9ca37d
Compare
|
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?
|
|
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. |
|
fixed in #1281 |
|
@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. |
|
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] #1282 |
|
I see, so you implemented what we discussed. Thanks for that. |
No description provided.