Dependencies: update requirement pyyaml~=5.4
#5060
Conversation
sphuber
force-pushed
the
fix/5059/pyyaml-dependency
branch
from
9bccf8c
to
f51915e
Compare
Codecov Report
@@ Coverage Diff @@
## develop #5060 +/- ##
===========================================
+ Coverage 80.24% 80.25% +0.01%
===========================================
Files 515 515
Lines 36753 36753
===========================================
+ Hits 29490 29491 +1
+ Misses 7263 7262 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Tests are failing because of conda install which is waiting for this release: conda-forge/plumpy-feedstock#54 |
| "pgsu~=0.2.0", | ||
| "psutil~=5.6", | ||
| "psycopg2-binary~=2.8.3", | ||
| "python-dateutil~=2.8", | ||
| "pytz~=2021.1", | ||
| "pyyaml~=5.1", | ||
| "pyyaml~=5.4", |
There was a problem hiding this comment.
Are we losing compatibility with the older versions? Otherwise it might be fine to leave this as is.
There was a problem hiding this comment.
This update was not so much for the compatibility, as it was to resolve critical security issues. The whole point of upgrading to 5.4 was that it fixes some critical safety problems and we shouldn't really use the older versions.
There was a problem hiding this comment.
I don't consider it our responsibility to force users to update 3rd party software as long as we don't prevent them from doing so due to our constraints.
That said, I'm fine with going ahead with this.
There was a problem hiding this comment.
Then what is the point of upgrading most of the requirements? Not trying to be snarky, really curious and trying to figure out if we should change our dependency management policy. We do this regularly, upping the minimum requirement, but I think that for many of the minor dependencies, we can just keep the lower level requirement and support a higher major version at the same time. Still we do this, but than surely we should definitely do this for upgrading minimum requirements for packages that pose potential security problems.
There was a problem hiding this comment.
There is in fact no point in updating our dependencies in this way in my opinion and if it is done in this way it is either not with my blessing or it was a mistake on my part.
We should make sure to update our requirements (the ones we test against and are specified in the requirements/*.txt files) and make sure that we are not incompatible with the latest versions, especially if those are fixing security fixes. Being compatible with PyYAML>5.1 was the primary point of this effort for me, not updating our dependency specification.
This policicy I've outlined would be in compliance with AEP 002 in my opinion.
There was a problem hiding this comment.
Just to come back to this, the reason why I thought what I described was the policy, is because that's what you did in these recent two commits: 6558c71 and 78ef633 In both you updated the minimum requirement in the setup.json. Was this because adding simultaneous support for the recent versions would have been to difficult?
There was a problem hiding this comment.
I think I had some reasoning when I made those changes (pytz uses calver and aldjemy was a major version bump), but the simple answer is that I simply didn't think it through when I made those changes.
There was a problem hiding this comment.
Could you also update
aiida-core/utils/requirements.txt
Line 3 in 6b8cf46
Earlier versions have critical security flaws that have been fixed in `pyyaml==5.4`. Note that `plumpy` also needs to be upgraded to `0.20.0` which adds support for this version of `pyyaml`. The `UnsafeLoader` is replaced by the `Loader` which are identical, but the former is only being kept as an alias for backwards compatibility but it might be removed in future releases.
sphuber
force-pushed
the
fix/5059/pyyaml-dependency
branch
from
e38811e
to
1e31e33
Compare
Earlier versions have critical security flaws that have been fixed in `pyyaml==5.4`. Note that `plumpy` also needs to be upgraded to `0.20.0` which adds support for this version of `pyyaml`. The `UnsafeLoader` is replaced by the `Loader` which are identical, but the former is only being kept as an alias for backwards compatibility but it might be removed in future releases. Cherry-pick: c78e0e2
Fixes #5059
Earlier versions have critical security flaws that have been fixed in
pyyaml==5.4. Note thatplumpyalso needs to be upgraded to0.20.0which adds support for this version of
pyyaml.The
UnsafeLoaderis replaced by theLoaderwhich are identical, butthe former is only being kept as an alias for backwards compatibility
but it might be removed in future releases.