-
Notifications
You must be signed in to change notification settings - Fork 76
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
Fix numpy version constraints #1012
Conversation
Hello @guillett ! Thanks for this fix proposal to #1009. The reason we can't easily pin NumPy to 1.20.x is that it will render the library unusable for the teams working at Institut des Politiques Publiques and Assemblée Nationale in France, because the latest available version they have at the CASD environment is 1.18.1. More generally, there's an ongoing discussion on how to properly handle dependency versions in #1010 (loose vd tight). #1010 provides a fix to #1009 that is not a major change but a minor one.
You mean to integrate this pull request and later #1010 to loosen up the constraint? |
There is also another point, which is why I removed the NumPy version pump from #1010 a couple of minutes ago:
|
The library is already unusable for IPP as of
Yes |
Yes! Thankfully the reason is already identified, tagged as a bug, and one possible solution is proposed in #1010. Regarding NumPy upgrades: you can see in #758, #990 and #924, that even if a lot of precautions are taken, sometimes the unforeseeable happens anyway. The difference is that now the library is unusable because of a bug. If we tighten the supported NumPy versions, we would also render the library potentially incompatible not just for IPP but for other users as well. We cannot easily know who's relying on which NumPy's version —specially in their own code, but we can as suggested in #924 check if the parts of the NumPy's API exposed by this library have been deprecated or not, in order to warn users of potential problems they could find. I have initially proposed in #1010 to actually contraint supported NumPy versions to the last four minors, but dropped it to propose that change in another specific pull request, as it would at a minimum be a minor change, and potentially a breaking change, when a bug fix is available as a patch. For that reason, even if I agree that we should improve the way we ensure actual usability of the library with the dependencies as they are declared in the |
Thanks again @guillett for this contribution 😃 The issue rendering OpenFisca-Core unusable for users with NumPy < 1.20 was fixed in #1010 🎉 Also, I integrated the changes you propose here in #1015 but taking into account the impossibility for some users to upgrade to NumPy 1.20 just yet and ensuring compatibility of new versions by testing both lower and upper bound supported versions of NumPy. Please do not hesitate to take a look and tell me what do you think. |
@maukoquiroga @guillett : shoud we close or merge this PR ? |
#1015 seems to need more discussion. This PR is small and seems to solve a real problem. If there is no side-effect (I had understood that some users depended on older versions of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok with the change from a technical standpoint. Tested locally with Country-Template and Extension-Template and all work.
Just unsure about potential consequences for reusers.
Considering the history of changing Numpy versions in #990, I'd be very reassured if some other reuser could confirm this is fine with them. However, I feel like approving this PR considering a reuser opened it initially.
Thanks @MattiSG, if I remembered correctly the @openfisca/france-contrib-ipp and the @openfisca/france-contrib-leximpact folks were locked on NumPy 1.17 (CASD). |
Hello, the CASD team could update the package if needed. They now deploy Anaconda with the Python version and packages we ask for. |
@MattiSG @benoit-cty : the CASD team can update but it is very costly for the IPP team to update now a,d very risky. |
@guillett @MattiSG @benoit-cty : I agree with @benjello . Currently, there are already strong moves through the harmonisation of legislation parameters in openfisca-france, which will ask for important adjustment on the repo taxipp. As we enter in a period a annual budget evaluation, we cannot support too much moves. And a change in the numpy version may have implications for the repo taxipp. I did not get the exact reason of this update, but unless it solves a blocking issue, I prefer the setup being stable. |
Thanks for your inputs. I'm a bit lost now. I understand that OpenFisca currently advertises being compatible with @guillett could we please have an update from you on whether this changeset would solve any problem for your team today? @openfisca/france-contrib-ipp do you have any automated tests for “taxipp” that would enable us to check the compatibility with numpy 1.20? |
@MattiSG : thanks for your answer. Unfortunately, we don't have such an automated test outside of CASD. |
I think this changeset is no longer useful as the context of it (#1009) has already been solved. Also, since #1014, supported versions are
More generally I hope with #1030 that we'll be able to do a matrix test of Python 3.7+ and Numpy 1.18+ so to help reusers give a more accurate guarantee of OpenFisca's actually supported versions. 😃 |
Speaking for @openfisca/france-contrib-ipp : our automatic test is broken but the blame is on us. |
This PR has been advertised as superseded by #1014 and further refined by #1010. I believe that it having been open for over a year while having been made obsolete by other changesets is a sign that the “getting a broken environment” issue has been fixed 😉 I'll thus close this PR. Feel free to reopen if I it is still relevant! |
When I update working environments without specifying explicit version but avoiding only major version change I get a broken environment with
since #990
Technical changes
Fix numpy version constraints
Can that be a patch version change to
35.3.8
that would be followed by #1010 that would also by a non major version change and that would untied the limit on numpy version ?