Skip to content
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

Remove Strict Upper Version Limit for Django #755

Closed
Mogost opened this issue Jun 12, 2024 · 24 comments · Fixed by #792
Closed

Remove Strict Upper Version Limit for Django #755

Mogost opened this issue Jun 12, 2024 · 24 comments · Fixed by #792

Comments

@Mogost
Copy link
Contributor

Mogost commented Jun 12, 2024

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:

  • Issue #680: Users requested compatibility with Django 5.0, highlighting the problem of strict version constraints.
  • Issue #698: Similar concerns were raised about the need for Django 5.0 support.

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
Copy link
Member

auvipy commented Jun 12, 2024

is django 5.1 released yet? or any alpha yet?

@Mogost
Copy link
Contributor Author

Mogost commented Jun 12, 2024

is django 5.1 released yet? or any alpha yet?

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

@auvipy
Copy link
Member

auvipy commented Jun 13, 2024

thanks, that is enough!

@auvipy
Copy link
Member

auvipy commented Jun 13, 2024

@Mogost
Copy link
Contributor Author

Mogost commented Jun 13, 2024

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
Copy link
Member

auvipy commented Jun 13, 2024

we can do that in future, but in the mean time #756

@cclauss
Copy link
Contributor

cclauss commented Jun 16, 2024

This is a repeat of the discussion at #680 (comment) 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
Copy link
Contributor

cclauss commented Jun 25, 2024

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.

@cclauss cclauss closed this as completed Jun 25, 2024
@Mogost
Copy link
Contributor Author

Mogost commented Jul 2, 2024

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

@cclauss
Copy link
Contributor

cclauss commented Jul 3, 2024

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
Copy link
Contributor Author

Mogost commented Jul 3, 2024

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

@noxan
Copy link

noxan commented Aug 20, 2024

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
Copy link
Contributor

cclauss commented Aug 20, 2024

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
Copy link
Contributor Author

Mogost commented Aug 20, 2024

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 #761 (comment) (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
Copy link
Member

Nusnus commented Aug 22, 2024

@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.

cclauss added a commit that referenced this issue Aug 22, 2024
Fixes #755

We agree to have a full test run before progressing to the next major versions of Django.

* #703 is an example of the kind of crash bugs that can be introduced in Django's major version upgrades.

* #755 describes why we should have confidence in Django minor and micro version upgrades.

Let's work together to add new versions of Django to our GitHub Actions as they are released to provide our users with safe upgrade paths.
@Mogost
Copy link
Contributor Author

Mogost commented Aug 22, 2024

@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 (#761 (comment)) 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 #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
Copy link
Contributor

cclauss commented Aug 22, 2024

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
Copy link
Contributor Author

Mogost commented Aug 22, 2024

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
Copy link
Contributor

cclauss commented Aug 22, 2024

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

@Mogost
Copy link
Contributor Author

Mogost commented Aug 22, 2024

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
Copy link
Member

Nusnus commented Aug 22, 2024

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
Copy link
Contributor Author

Mogost commented Aug 23, 2024

@Nusnus this issue was not resolved

@Nusnus
Copy link
Member

Nusnus commented Sep 21, 2024

@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?

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

Successfully merging a pull request may close this issue.

5 participants