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

Loosen or update cryptography requirement for dash[testing] #1466

Closed
mikemucc opened this issue Nov 24, 2020 · 6 comments · Fixed by #1506 or mstoelzle/solving-occlusion#28
Closed
Milestone

Comments

@mikemucc
Copy link

Thank you so much for helping improve the quality of Dash!

We do our best to catch bugs during the release process, but we rely on your help to find the ones that slip through.

Describe your context
dash[testing] has Cryptography hardcoded to 3.0 (cryptography==3.0). Can we either get this loosened or updated to 3.2.1 as cryptography > 3.2 has a vulnerability.

Describe the bug

dash[testing] has Cryptography hardcoded to 3.0 (cryptography==3.0).

Expected behavior

Can we either get this loosened or updated to 3.2.1 as cryptography > 3.2 has a vulnerability.

@anders-kiaer
Copy link
Contributor

Semi-related to #1457 (wrt. generally loosening pip install dash[testing] dependency versions).

@alexcjohnson
Copy link
Contributor

We can certainly update this requirement (PRs welcome 😄)

Loosening these requirements is also a good idea, but will take a bit more effort. As @anders-kiaer deduced in #1457, the purpose of pinning was originally for our own reproducibility in CI testing, so if we are to loosen these requirements we need to either simultaneously pin them in the CI setup for dash and our other repos that use it (the core dash-core-components, dash-html-components, dash-table, and others dash-bio, dash-daq, etc) or find a good way to respond to any failures that might creep in due to dep upgrades.

If we did decide to pin internally but loosen [testing], that could take the form of another requirements file in the main dash repo that's used just for this purpose, or perhaps we just pack the pinned versions of all of these into [dev], so that [testing] is for use by other projects and internally we only use [dev]? The problem I see with that is that we won't know about problems in [testing].

@anders-kiaer
Copy link
Contributor

As @anders-kiaer deduced in #1457, the purpose of pinning was originally for our own reproducibility in CI testing, so if we are to loosen these requirements we need to either simultaneously pin them in the CI setup for dash and our other repos that use it (the core dash-core-components, dash-html-components, dash-table, and others dash-bio, dash-daq, etc) or find a good way to respond to any failures that might creep in due to dep upgrades.

dash + dash[testing] probably have more indirect Python dependencies than direct dependencies. I.e. even if we pin the direct test dependencies, CI failures will happen due to indirect dependency upgrades. A recent example in our project earlier this autumn was pylint failure due to new astroid version (even though there was new pylint version released on PyPI at that time).

If you want to pin all dependencies (indirect + direct) maybe pipenv could be an option to use during CI, and let it fail when lock file is out of sync. My experience with pipenv is limited, especially in CI setting. You will still have the challenge as you mention @alexcjohnson that CI then would not see challenges users meet - i.e. you would maybe like to double the number of CI jobs (pinned + unpinned, and let the unpinned jobs be allowed to fail).

In our Dash based project, we have gone for a much simpler mitigation route: We have scheduled CI runs (once every night), and also pip freeze as part of the CI run. This way maintainers + community immediately will see when a new upstream dependency breaks something (maintainers could also get an email notification regarding failed scheduled master). Quick notification, and the possibility of comparing with last success' pip freeze quickly identify the new release (either direct or indirect dependency) causing the failure, and we are able to also notify the upstream dependency quickly after they new release (which they typically are grateful for, since the dependencies in question here are typically well maintained and with long deprecation cycles, i.e. a new release causing a failure is often unintentional and not a sudden breaking change).

I'm not familiar with Circle CI (and actually had some trouble getting Circle CI to start in my Dash PRs). Is it correct that PR authors would need a Circle CI account to be able to start and/or see runs? We recently jumped over to GitHub actions and happy with it so far (example of scheduled runs in a Dash component repository). Not sure if GitHub actions is something that has been considered in Dash, but I'm happy to contribute with some CI workflow suggestions if that is the case. Then #1440 can maybe be addressed at the same time if found useful (automatic deploy of sdist + wheels to PyPI when Dash maintainers do a release on GitHub).

Side note: I just observed that GitHub apparently does not pick up the requires-*.txt files in this repository. At least I can not see them in https://github.com/plotly/dash/network/dependencies. Might be that it picks up requirements-*.txt, and thereby give maintainers of Dash the option of being notified regarding dependency security advisories (like in the issue reported here).

@ned2
Copy link
Contributor

ned2 commented Dec 3, 2020

This is perhaps something of a more pressing concern as it seems that dash[testing] just on its own now has an unresolvable set of dependency constraints since pyopenssl recently bumped to cryptography>=3.2 in v20.0.0 released on November 28.

pip install dash[testing]   

>>> pyopenssl 20.0.0 requires cryptography>=3.2, but you'll have cryptography 3.0 which is incompatible.

More generally, I agree that it would be great if dash[testing] could be unpinned/loosened. As @anders-kiaer mentions in #1457, dash[testing] is advertised as a (super useful) public API, and pinning runs counter to that purpose.

@alexcjohnson
Copy link
Contributor

dash + dash[testing] probably have more indirect Python dependencies than direct dependencies.

@anders-kiaer that seems like the winning argument to me 🏆 especially with the recent pip resolver changes.

So we'll loosen all the requirements in [testing] to >=

We'll keep [dev] requirements == because this is used solely (AFAIK) by our own CI - on this repo, the rest of the dash core, and other dash components such as dash-daq and dash-bio.

@alexcjohnson
Copy link
Contributor

Just for future reference: cryptography is indeed one of these indirect dependencies, it's not used directly at all. We pinned it in the summer due to a windows bug, see #1384 (comment). Looks like that has been fixed so hopefully now we can simply remove it again from our requirements 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants