Skip to content

Conversation

xmatthias
Copy link

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests
  • Added myself alphabetically to AUTHORS.rst (optional)

cachetools version should not be hardpinned - as this will prevent projects depending on this library from upgrading dependencies.

@Poolitzer
Copy link
Member

Wouldn't the same argument apply to APScheduler? Why did you only make the change for cachetools?

In general, there is an issue with dependency python has: If one does not hard pin the version, breaking changes in a dependency can make our library code break. If we hard pin the version, we force our users to miss out on (security) updates to the libraries, and they cant have a higher version in their projects then the one we hard pin, which seems to be your case.

I don't know the reasoning behind hard pinning two libraries right now, having two at "higher then this version" and have one at no version at all, another maintainer will be able to shed light on that.

@xmatthias
Copy link
Author

In general, there is an issue with dependency python has: If one does not hard pin the version, breaking changes in a dependency can make our library code break.

In general, the usus / best practice in python is as follows (at least that's what i understand from past research):
hard-pin dependencies in requirements.txt - so CI can run on a deterministic version.
Obviously, things like dependabot help in updating this - which will help identify breaking changes (so the below can be applied selectively).

in setup.py - dependencies should be loosely pinned (this is what's used when running pip install <library>) - covering all supported versions.
To protect against future breaking changes, a version pin like cachetools>=4.2.2,<5 can be made - assuming the library adheres to best practices, and increments the major version number for breaking changes.

Now this creates a different maintainability problem - as now every dependency has to be defined twice (once in requirements.txt, once in setup.py ... but it's the best approach I've come across (for libraries).

Wouldn't the same argument apply to APScheduler? Why did you only make the change for cachetools?

Yes that's correct, but that's not an immediate problem i encounter - but i'm more than happy to change it for this as well if you would prefer this.
An alternative approach would've been to simply increment cachetools version to 4.2.4 - but that would then force every project depending on telegram-bot to upgrade - which is for sure not ideal.

@Poolitzer
Copy link
Member

Very good point you made, thanks. Lets see what the other maintainers have to say, maybe we can come up with a solution.

@Bibo-Joshi
Copy link
Member

Hi. Thanks for the input @xmatthias !

This is definitely something that we should have a more clear policy on. So I don't want to make any quick call on this. We'll have to evaluate the different possibilities and double check what we're comfortable with.
For now, I'll put this on the v14 milestone so that this will be discussed before our next major release :)

@Bibo-Joshi Bibo-Joshi added this to the v14 milestone Oct 30, 2021
@xmatthias
Copy link
Author

This is definitely something that we should have a more clear policy on. So I don't want to make any quick call on this. We'll have to evaluate the different possibilities and double check what we're comfortable with.
For now, I'll put this on the v14 milestone so that this will be discussed before our next major release :)

For a final solution, i tend to agree.
however telegram-bot is right now forcing an outdated version of cachetools - so i'd expect a near-term solution for the next minor release (even if it's not my proposed solution).

@Bibo-Joshi
Copy link
Member

Let me clarify: We're not planning to make any minor release before v14 (unless there is an API update). See also the last paragraph of the v13.7 release notes.
In the meantime, you can still install a newer version of cachetools after installing ptb. Running

pip install python-telegram-bot
pip install cachetools>=4.2.2 -U

the second command will give you a warning along the lines

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed.
This behaviour is the source of the following dependency conflicts.
python-telegram-bot 13.7 requires cachetools==4.2.2, but you have cachetools 4.2.4 which is incompatible.

but it will still install cachetools 4.2.4 :)

@xmatthias
Copy link
Author

In the meantime, you can still install a newer version of cachetools after installing ptb.

This will for sure work for me personally - but is unsuitable to be used by an application where we'll have to support general installation, where ptb is only a dependency, not the main tool.

@Bibo-Joshi
Copy link
Member

I see your point. However, you're the first persion I know of that asks to upgrade the cachetools dependency (and I only recall 1 case where someone asked about APScheduler). So if the "update cachetools after installing PTB" strategy works out for you, I'd prefer to make a well prepared change for v14 instead of making a quick call for master that might end up never to be included in a release :)
Note that another workaround could be to vendor PTB in order to change the dependencies …

@xmatthias
Copy link
Author

xmatthias commented Oct 30, 2021

So what's the release date of v14?
As you keep pointing to that version, i assume you have a date for this set which you aim at, but so far, i've only seen notes about "maybe in a week, maybe in a few months" about v14.

With your comments here (not going to upgrade dependencies to the 13x branch) i assume it'll only be days (maybe 2-3 weeks) out until v14 is ready (that would however also make me assume you'd have an alpha version published?).

Otherwise, i find your statements unacceptable and quite strange, as i'm sure you'll forget about this version-bump should you have to do another release in the 13.x branch.

@Bibo-Joshi
Copy link
Member

So what's the release date of v14? As you keep pointing to that version, i assume you have a date for this set which you aim at, but so far, i've only seen notes about "maybe in a week, maybe in a few months" about v14.

I don't know. You apparently have found this announcement from which it should be clear that v14 is a big project and that we don't like to give ETAs.

With your comments here (not going to upgrade dependencies to the 13x branch) i assume it'll only be days (maybe 2-3 weeks) out until v14 is ready (that would however also make me assume you'd have an alpha version published?).

Then you've misunderstood me. What I said was that if you're the only one so far who has problems with the pinned cachetools dependeny and are okay with my proposed workaround, then I don't deem a change in the dependency management necessary for the 13.x branch.
If there is a significant number of people who also have problems with our current dependency management for whom it also is not an option to wait for a v14 release or to use the proposed workarounds, then I'm happy to make a corresponding thought-trough change to the 13x branch instead of to the v14 branch. Whether or not this change would lead to an immediate release is another question.

Otherwise, i find your statements unacceptable and quite strange, as i'm sure you'll forget about this version-bump should you have to do another release in the 13.x branch.

Rest assured that the organizational skills of this libraries developer group are quite sufficient to keep track of the changes that we want to include in a release.

Our policy on the v13 branch is basically the following:

  • API updates go into v13
  • bug fixes go into v13 because they should be included in case we make a release for an API update
  • everything else goes into v14 unless there is another compelling reason to put it into v13.

This policy is in place to make development of v14 easier, hence faster.

For the reasons mentioned above I don't (yet) see a compelling reason to change the dependency management on the v13 branch.


It's not clear to me what your aim in this discussion is: You confirmed #2757 (comment) that my proposed workaround #2757 (comment) works for you. So I'm wondering what you'd gain from a release including the change coming sooner than later, if you need to employ the workaround anyway, until such a release drops.

@xmatthias
Copy link
Author

xmatthias commented Oct 30, 2021

You apparently have found this announcement from which it should be clear that v14 is a big project and that we don't like to give ETAs.

I do understand this, and wouldn't do this either - but without an ETA, i don't think it's justified to no longer fix propblems that appear in v13.

Our policy on the v13 branch is basically the following:

This sounds like you've just invented it - otherwise you'd not write it down here, but link to where the so-called policy is documented, clearly, for everyone to see.

It's not clear to me what your aim in this discussion is:

The workaround works for me personally - but does not work for me as a project maintainer of a project depending on ptb.
I cannot tell every one of my users (6000+, based on the discord server size) to not run pip install xxx - but instead run pip install python-telegram-bot - followed by pip install xxx (the project itself is not relevant).

It's the same with vendoring - that's an unreasonable approach, and seems to only try and "push away" the problem without looking for a fix.
It's also one that i only deem acceptable for a library that's no longer maintained (or refuses certain fixes a project requires, for whatever reason), as it moves the maintenance burden of that library/project to the project vendoring it.

I can throw this question back at you, too.
In the time it took you to try and argue my PR away, you could've merged and created a minor release bumping the version very easily (assuming you have Automation setup properly), so it's apparently unwillingness from your side, not technical reasons.

Also a proper resolution to this can be found and merged to v14 - and immediately backported to the existing version, so a potentially necessary release will include this - but that would assume the will to do so.

that my proposed workaround #2757 (comment) works for you

I confirmed that the approach may work for me from a conceptual perspective (so i thought it may fix it for myself, but not for the project i'm working on) - but indeed after trying the fix myself, it will not work with uptodate pip versions.


I've understood now that you do not aim to accept this PR, but rather like arguing - so i'll convert this into an issue, where you can find a proper fix.


I'd also urge you to stick to your own code of conduct here:

Focusing on what is best for the community
Showing empathy towards other community members

I don't think it's good for the community to try and push people trying to fix a detected problem away, but should rather try to understand the problem first.
It's also not good to no longer accept problems in your current version (even if they were not discovered before this) - but in the end, it's your community, and based on my experience here, i'm not going to try and fix this myself, but will rather stick to opening issues should i find some.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants