-
Notifications
You must be signed in to change notification settings - Fork 135
Allow compatible dependency versions #1445
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
Conversation
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.
Frankly the range of changes here is a bit diffuse for a single PR. I agree with the title that the changes to requirements seem to be the most consequential. I think they are potentially problematic for two reasons:
- having ranges for so many packages means that pip will potentially have to spend a LOT longer trying to find compatible versions for everything
- this is a bit less concerning, but in practice you can't always rely on major versions to prevent breaking changes. pinning exact versions is a way to make dependencies more reliable
If people are having issues with the package versions, we already have a solution for that - the friendly dependencies install allows for more liberal package versioning (defined in setup.py). there are tradeoffs with pinning exact versions, and the current setup allows users to choose which side of the tradeoffs they prefer (friendly dependencies or not). this PR gets rid of that choice.
I think it could still be valid, but it's a consequential package design choice point.
|
I tend to agree (and would generally just set this to a recent version of each with the next minor version as the max), but wanted to demonstrate the diverse range of dependency versions which pass all tests. |
|
Regarding the pip install times, uv installs it very quickly in a brand new venv, but you're right that the pip install takes a while. It takes just over twice as long to install. |
7693a62 to
8a2ace3
Compare
|
I took the changes that weren't necessary to the primary purpose of this PR and moved them to new PRs |
|
It seems like parsons limited dependencies/extras doesn't work with uv for some reason, which makes me more partial to this PR, although it's still a bit indirect. Seems like we need a comprehensive fix for dependency management. But yeah in the meantime this PR would be helpful for using uv with parsons. |
|
That is so strange because we do test limited dependencies in our GitHub Actions workflow... |
|
ahh you're right i'm probably just doing something wrong |
8a2ace3 to
eabb23d
Compare
eabb23d to
3e7b489
Compare
|
what about ditching the requirements file entirely and doing something like this in setup.py def main():
install_requires = [
"petl",
...,
"simplejson",
]
extras_require = {
"airtable": ["pyairtable"],
...,
"twilio": ["twilio"],
}
extras_require["all"] = sorted({lib for libs in extras_require.values() for lib in libs})
limited_deps = os.environ.get("PARSONS_LIMITED_DEPENDENCIES", "")
if limited_deps.strip().upper() not in ("1", "YES", "TRUE", "ON"):
install_requires.extend(extras_require['all'])
extras_require = {"all": []}that way you are only needing to manage dependencies in one place? |
Personally, I'm very hesitant to use that approach. I do like the functionality though! |
|
Right now, the two ways to install all the dependencies will install different sets of dependencies.
and
will not install the same exact set of requirements, because this does not seem good. if we are very, very careful to keep the requirements.txt file up and the setup.py extras_require up to date we may be able to have the two ways of install all the dependencies install the same, but that seems actually very hard. I agree that our setup.py is strange, and I look forward to world where we don't need to support the dynamic behavior of PARSONS_LIMITED_DEPENDENCIES. I see my suggested change as a step torwards that. When |
|
I do think it is more idiomatic for something like the PARSONS_LIMITED_DEPENDENCIES behavior to be the default, and users always need to specify extras or [all] if they want them. This can all be achieved in pyproject.toml rather than setup.py or requirements.txt. Here's an example: pypa/setuptools#3627 (comment) |
What's Changed