159: Remove upper pins on package dependencies

[lsterk]

Upper requirements are primarily useful for avoiding versions which are known to be unsupported. It is also possible that a new version of an unpinned dependency could cause a regression in this package's performance. If this happened, any downstream users who upgrade to the newer dependency version might experience broken functionality in kafka-utils, which is bad.

One mitigation for this is to have upper pins on all dependencies, which is what kafka-utils has historically done. By explicitly forbidding yet-unreleased packages from being used, there's no worry that the kafka-utils performance will be broken by a newer dependency version. I do see the argument for proactively upper-pinning dependencies to prevent kafka-utils from breaking due to a new version of a dependency.

However, maintaining the proper upper requirements is hard, and takes a lot of effort. In order to not have fairly strict (and seemingly arbitrary) requirements on what downstream users can do, the maintainers of kafka-utils would need to create and test a new patch version release which bumps the version requirements every time a new dependency version is released. My guess is that this isn't something that the maintainers are interested in doing. Even pinning just major versions (i.e. <NEXT_MAJ_VERS.0.0 ) for every dependency takes some work... and the process ultimately becomes an unending battle to react against the actions of progress in your dependent package. This doesn't sound fun for maintainers of this package, and also doesn't place a lot of faith in the developers of packages that you rely on.

My suggestion (and the philosophy behind this PR) is to have trust in the developers working on the dependencies of kafka-utils and rely on them to not make breaking changes unless totally necessary. If features that are used in kafka-utils are going to be removed, we should be able to trust that their removal would be prefaced by a deprecation warning (which should be caught while doing maintenance on this package). If it is discovered that the newest version of a dependency causes issues with kafka-utils, take action by updating the code of kafka-utils as necessary. These issues will likely surface quickly in CI pipelines, or perhaps as just an Issue reported by users.

At the end of the day, this package is only useful if it gets used, and if upper pins make it difficult to be used, it will cease to be useful. Most devs would probably rather have to update kafka-utils occasionally to pick up bug fixes than have to delay new features because kafka-utils doesn't play well with newer versions of other packages.

Happy to change instead to updating version pins to be "less than the next-unreleased-major-version" but I think this PR will be more effective long-term and make both maintainers and users of this package happier :)

This PR addresses the concerns of #159

[Baisang]

Thanks for doing this @lsterk!

I think for this kind of change we would also want a version bump (to 1.8.0 perhaps? I can just do this) and we would test it out internally in development environments first to catch any potential bugs. After that I'd be more confident in rolling this out everywhere.

Does that seem reasonable?

[lsterk]

(Sorry for the delay!)

@Baisang I think a patch version bump is what would be expected, given that this is not adding a new interface - it is merely changing some requirements of the package. But, I don't have terribly strong feelings about this - once my changes get merged in, they can be released via whichever type of version bump you prefer :)

Re/ testing: seems legit to test in a dev environment first. The only testing I was able to do was the test suite run by CI.

[lsterk]

@Baisang I could also add a change to address #146 by adding a minimum version to the paramiko requirement (i.e. paramiko>=1.8.0). Would this be reasonable to include in this PR?

[Baisang]

@lsterk yeah, might as well include that as well. Thanks again for your help.

[ghost]

I think another solution for potential impact in prod is add something like https://jenkins-paasta.yelpcorp.com/view/services-service_errors_monitor/ to the kafka-utils internal pipeline. As we can block build from goto prod in stage by setting manually tigger prerequisite before go to prod. Then we can do manual tests in those stage boxes when we want to push new versions to prod. The benefits are: Only need to do tests when we want to push new changes and always get newest dependency version in dev and stage pipeline boxes, quickly know issues. The disadvantages are obvious as well: pipeline build in dev, stage may fail frequent, as always try to use newer dependency version, possible to have compatible issue etc.

[akki]

Although I agree that it might be a good idea to remove the strict requirements (as mentioned in #159), I think fully removing the upper pins might be disastrous when new major versions are released in upstream repositories.

For instance, currently, the requests module is pinned at the major version 3 (i.e. 3.0.0) as the current version is 2.21.0, because the module would most likely start releasing under 3.0.0when there are breaking changes.
So whenever requests=3.0.0 (or any of the other dependencies) is released with breaking changes, anyone who installs kafka-utils will find it broken.

So the changes to requirements.txt look useful but maybe the changes in setup.py should be skipped? Am I missing something here?

[lsterk]

@akki I agree with your concerns. There's a balance to be struck here, though:

If the maintainers of this package want to take a reactive approach to all upstream package changes, keeping the upper pins will protect against future breaking changes. When a new upstream package has a breaking change, the maintainers here can evaluate the changes, and create a PR to bump the max version to the next major version (e.g. if request==3.0.0 comes out and it works fine, just file a PR to bump the setup.py to declare requests<4.0.0). If there are logic changes needed to accommodate the breaking changes, those can be introduced in the same request.

If the package maintainers don't want to commit to making a new PR every time that the upstream package is generated, they can adopt this PR, which basically says that the dependencies of this project seem stable enough and that we can risk taking a breaking change when possible.

I'm happy to adjust this PR to keep the upper pins, but bump them for any package that has a new major version release available.

[lsterk]

Closing this in favor of #270 as most of the version pins have been addressed in separate reviews.