celery / django-celery-beat

Celery Periodic Tasks backed by the Django ORM
Other
1.68k stars 427 forks source link

Remove Strict Upper Version Limit for Django #755

Closed Mogost closed 1 month ago

Mogost commented 3 months ago

Hi Maintainers,

I am writing to request the removal of the strict upper version limit on Django in the django-celery-beat library, with the added bonus of ensuring compatibility with Django 5.1.

Background:

Currently, the library enforces a strict upper limit on the supported versions of Django. This has previously caused issues for users wanting to upgrade to newer Django versions, as noted in the following issues:

Arguments for Removing the Upper Version Limit:

  1. Increased Flexibility: Removing the upper version limit will provide users with the flexibility to upgrade to newer versions of Django as soon as they are released, without waiting for an official update from django-celery-beat.

  2. Accelerated Compatibility Testing: By allowing the installation of the library with newer Django versions, the community can quickly test and report any issues, facilitating faster updates and fixes from the maintainers.

  3. Clear Compatibility Declaration: While removing the upper limit doesn't implicitly declare support for all future Django versions, it allows developers to experiment and provide feedback, thereby contributing to a more robust testing process.

  4. Community Feedback: There has been consistent feedback from the community about the inconveniences caused by the strict version constraints. Addressing this would significantly improve the developer experience for a wide range of users.

Proposal:

  1. Remove the upper version limit for Django in the setup.py and tox.ini files.
  2. Ensure compatibility with Django 5.1 by running the test suite and making any necessary adjustments.

Thank you for considering this request. Your efforts in maintaining this essential library are greatly appreciated by the community.

auvipy commented 3 months ago

is django 5.1 released yet? or any alpha yet?

Mogost commented 3 months ago

is django 5.1 released yet? or any alpha yet?

yep. https://pypi.org/project/Django/5.1a1/ alpha

auvipy commented 3 months ago

thanks, that is enough!

auvipy commented 3 months ago

the upper limit is relaxed already https://github.com/celery/django-celery-beat/blob/main/requirements/test-django.txt

Mogost commented 3 months ago

the upper limit is relaxed already main/requirements/test-django.txt

No. it is not. https://github.com/celery/django-celery-beat/blob/816d6ab878b3826462e47a00a28c148783e74a8e/requirements/runtime.txt

And relaxing is not enough. This upper version restriction should be removed at all.

auvipy commented 3 months ago

we can do that in future, but in the mean time https://github.com/celery/django-celery-beat/pull/756

Mogost commented 3 months ago

This https://github.com/celery/django-celery-beat/blob/a9a6465f8ae0ea8753cd10bd96913b34259232fe/requirements/test-django.txt and this https://github.com/celery/django-celery-beat/blob/dd293477c986922e7a8059298f2b10bb6d205a25/requirements/runtime.txt are different.

This upper limit has no real benefit. Can we remove it now?

cclauss commented 3 months ago

This is a repeat of the discussion at https://github.com/celery/django-celery-beat/issues/680#issuecomment-1939281880 and I would use the same arguments that I used there to resist this change. django-celery-beat was crash-broken on Django v5 so a Clear Compatibility Declaration is something that maintainers should not give before the software combination has been tested.

cclauss commented 3 months ago

This upper limit has no real benefit. Can we remove it now?

Nonsense. It allows us to test software before users put it into production.

Mogost commented 3 months ago

@cclauss @auvipy Might this article change your opinion https://iscinumpy.dev/post/bound-version-constraints/?

cclauss commented 3 months ago

Please help with #761 where testing on a beta version of Django downgrades both django-celery-beat and its dependency django-timezone-field. If we had not tested before our users did then they may have used out-of-date versions in production. Let's find and fix the root cause of these downgrades before we consider boundless versions.

Mogost commented 3 months ago

@cclauss I just answered you in the PR. But please reopen this ticket. Problem of this ticket is not resolved

noxan commented 1 month ago

how about restricting by major release version (<6.0)? django has a very sophisticated deprecation and release process, and only major versions should introduce breaking changes.

cclauss commented 1 month ago

~What happened when Django Celery Beat clients upgraded from v4.2 to v5.0? Perhaps go through the issues and pull requests to see the crashing bug at that time.~

Oh, I misread. I would agree to restricting by major release version (<6.0).

Mogost commented 1 month ago

I just want to refer one more time to @henryiii's blog post. Usually, I rely on @adamchainz blog for best practices in Django world but this time I have not found a good guide (just mentioned Adam, probably this might be a new post for the Django world). I'm sorry if I'm being too intrusive.

@cclauss In the situation you mentioned there were nothing except django-celery-beat wrong version binding.

What happened when Django Celery Beat clients upgraded from v4.2 to v5.0? Perhaps go through the issues and pull requests to see the crashing bug at that time.

Here and previously there is no actual problem with compatibility. There is only one restriction from the django-celery-beat. This time is just the same situation as we found here https://github.com/celery/django-celery-beat/pull/761#issuecomment-2206113127 (In fact, our dialogue there only confirms that you yourself are already confused by your own restriction)

A lot of the commentators in #680 @ddahan @deronnax @nlsfnr have been basically ignored.

I honestly don't see any potential compatibility issues for django-celery-beat due to django. But at the same time I wouldn't argue against the celery version restriction (it's a really weighted restriction).

I'm not sure if I can reach your heart, but this restriction really upsets me.

Nusnus commented 1 month ago

@Mogost

I'm not sure if I can reach your heart, but this restriction really upsets me.

Hey there - I don’t want to intervene so much in the discussion but this part touched me. Please, don’t let your professional discussion with @cclauss upset you. He is a professional, and I am 100% sure he is not looking into any sort of “fighting”. We are all allowed to debate professional opinions and @cclauss is currently leading the django-celery-beat project so please remember he is just being responsible and I am sure that if you wish to change his mind with a sound argument he’ll be willing to listen.

Please, don’t take it to heart. We care to hear what you want to say, but let's be calm and professional.

Mogost commented 1 month ago

@Nusnus I don't see the discussion (it looks more like I want restriction - I will keep it). In previous discussions a large number of professionals have spoken out and as it seems to me were not heard. And I'm still trying to make the point, as a professional, that this restriction is unnecessary.

The argument we had a problem in the past so we keep the restriction is not valid, because what is described as a problem was not a problem at all.

This time you have re-noticed a problem that didn't exist. I think in this comment (https://github.com/celery/django-celery-beat/pull/761#issuecomment-2206082326) I have clearly shown that the problem does not exist. It is the check itself that is invalid.

And understand, I appreciate the product and your contribution to it, but in this case I as a user suffer more from this restriction. Especially when this restriction is meaningless and there is no real compatibility problem. I as a developer can limit the version of anything in my project if I see problems, but I can't influence pip and ignore the limitation imposed by the library (actually you can make a fork, but that's not really the right approach).

Please help with https://github.com/celery/django-celery-beat/pull/761 where testing on a beta version of Django downgrades both...

I was asked to help, I came and helped. And I see that you yourself are suffering from your own restriction.

I would agree to restricting by major release version (<6.0).

It doesn't remove the problem. It just makes it more rare. (not every release, but every major release) As a solution, I would suggest running tests on django-(main/upstream) like many projects do.

Also django does not respect typical semver. https://docs.djangoproject.com/en/dev/internals/release-process/#release-cadence You can get backwards incompatibility even in a minor semver change.

cclauss commented 1 month ago

I would suggest running tests on django-(main/upstream) like many projects do.

Nice! A pull request to add this to .github/workflows/test.yml would be warmly received.

Mogost commented 1 month ago

I would suggest running tests on django-(main/upstream) like many projects do.

Nice! A pull request to add this to .github/workflows/test.yml would be warmly received.

what about the restriction? Would you remove it after that?

cclauss commented 1 month ago

Can we prove that this repo has extremely high test coverage?

Mogost commented 1 month ago

Can we prove that this repo has extremely high test coverage?

Whatever coverage you have, even if it's 100 percent, it will never be 100 percent in reality. There's always a chance that something will go wrong. A developer bumping a Django version on his project is usually prepared for such problems/incompatibilities. Sometimes he can bypass them on his end, sometimes he can wait for the maintainers to act, sometimes he can do PR and help the maintainers. It is often after the release of a new Django that open-source libraries get reports of problems. Sometimes even a ready PR with a solution. Here django-celery-beat has a soft power by declaring classifiers what is tested and supported. Restriction about celery version is right because django-celery-beat is highly connected with celery and compatibility has to be checked carefully. But I don't see anything too special regarding django here. Most of the updates will probably be without a single change other than bumping this restriction.

So I think testing with djang-main + failing tests on warnings (-Werror) would be enough.

Nusnus commented 1 month ago

Can we prove that this repo has extremely high test coverage?

I’ve fixed the code coverage configurations for this project in #793 so we can know for sure now. CleanShot X 2024-08-22 15 27 17

Mogost commented 1 month ago

@Nusnus this issue was not resolved

Nusnus commented 2 weeks ago

@Nusnus this issue was not resolved

Hey @Mogost, I apologize for the delay; I was very busy the last few weeks. What is the remaining issue you’re dealing with, and how may I help you?