-
Notifications
You must be signed in to change notification settings - Fork 2
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
CKAN Coding Standards -- Dependencies #270
Comments
Thank you for this Eric. We've arrived at our current imperfect state of dependency management after struggling with some of the ideas you present above. Semantic versioning is a nice idea, but many dependencies or sub-dependencies don't maintain compatibility across versions the way you would expect. Without pinning versions in requirements.txt we have frequent build errors unrelated to our own changes. Reproducible builds are a really good thing.
The extra work of determining the safe range of requirements for every dependency or sub-dependency is significant, potentially taking away time from developing features our users need. Right now that extra work falls to the end users or system integrators building and supporting CKAN sites with many extensions and customizations. Those users only need to make their extensions and customizations work for a single set of versions they care about and are able to test themselves. There's room for interested parties to package CKAN+extensions with sets of known working package versions that are easy for end users to install. In fact they already do as docker images, amazon images etc. I think we should support their efforts and maybe even link to well-maintained examples from our install instructions. AFAICT pip isn't the right tool for this problem. |
Python packaging is complicated. At least we're not trying to ship C-extensions. But, it is getting better. The 2020 resolver in pip is way better at finding a solution that will match a large number of libraries, as long as it can get the data and they're not too picky about the exact versioning of the requirements. The previous resolver had a lot more issues finding a workable solution. Right now, we have the worst of both worlds. We have a lot of potentially conflicting requirements, we're not notified of them (pip happily uninstalls one and installs another, potentially older version), and we can't even use the existing tools to make some sort of resolution. So to get to the point that we have a installation that qualifies for a reproducible build, we've got to go through, find, and manually resolve conflicts. (and then pin them) Most other python packages (including the requirements) are installable from pypi, and specify a set of requirements in
And The google api client:
And the Office rest client:
In contrast, there are the ckan ones: (of these, scheming is the only one that specifies dependencies upstream, the other two I've patched)
Note that the ckan ones don't help me by specifying dependencies. Looking at this install, I've got this many transitive dependencies of six, not including ckan and extensions:
Without the install_requires, beautiful_soup is a top level dependency:
But unfortunately, grepping through the requirements.txt files, this is what I have for beautiful soup:
This happily installs, and depending on the order, I get something. If these were in the install_requires, I'd at least get notified. Thankfully, looking at all of these, there are only a couple of silent conflicts, but they'd be better if they were loud. While semantic versioning isn't going to save us, it is a tool. It's better than the requirements.txt, where the only guarantee is that a package matching that name will be installed. Tests can save us, if they have enough coverage and get run. (sorry for the length, the copy/paste of pipdeptree was longer than I'd thought) |
CKAN version
All currently shipping
Describe the bug
The current CKAN coding standards for extensions leads to inconsistent installs and works against the zen of the python packaging system.
Specifically: (https://docs.ckan.org/en/2.9/extensions/best-practices.html#add-third-party-libraries-to-requirements-txt)
Separating the dependencies from setup.py breaks the pip resolver. So not only are we likely to get conflicts, we can't even use the tools to figure out what they are.
Steps to reproduce
A small example:
I'm sure that there are other examples, e.g requests, six.
Expected behavior
Ideally, the ckan, extensions, and all of the python dependencies could be installed with:
pip install --use-feature=2020-resolver ckan ckanext_foo ckanext_bar ckanext_scheming ckanext_pages...
This would be a one-shot install, and the pip resolver could build the entire dependency tree, and inform us of conflicts, which could then be resolved.
Then, we could see the results of our dependencies with
pipdeptree
:Where now, CKAN has no dependencies. (note, ckanext-harvest has had it's requirements hoisted (and tweaked) into setup.py in my install.)
Additional details
To reduce the number of conflicts, extensions should be liberal in the versions what dependencies they pin, e.g. pinning a specific version of requests is unnecessary, especially given how stable the requests interface is.
Obviously, there are cases where a specific pinned dependency is required. That's to be expected if you are relying on the specific behavior of a dependency Transitive dependencies should not be pinned. (e.g., certifi and pytz have stable interfaces, and should generally always be on the latest version). Any extension dependency that's also a dependency of CKAN shouldn't be pinned, because then the CKAN pins for different major versions will conflict.
CKAN should be the most specific pin, and extensions compatible with a specific version of CKAN should be compatible with that pin. However, CKAN should also be generally forgiving of the exact version of dependencies that have relatively stable interfaces where it's not using 'deep' features.
Ideally, CKAN would also be only pinning direct dependencies to make it easier to have a clear view of what needs to be updated on point releases. But that's a further bridge to cross.
The text was updated successfully, but these errors were encountered: