Skip to content

remove django from base requirements#64

Merged
UsamaSadiq merged 1 commit intomasterfrom
usamasadiq/remove-django-requirement
May 18, 2021
Merged

remove django from base requirements#64
UsamaSadiq merged 1 commit intomasterfrom
usamasadiq/remove-django-requirement

Conversation

@UsamaSadiq
Copy link
Copy Markdown
Member

@UsamaSadiq UsamaSadiq commented Feb 23, 2021

JIRA Issue: BOM-2418

Details:

Moving the Django requirement from base.in to django.in because:

  • Django requirement in base.in causes the django version in testeng-ci repo to upgrade to django==3.1.6 which causes failures in different jobs.
  • It upgrades the django version to 3+ in any repo that uses this package without applying constraint on django versions.

@UsamaSadiq UsamaSadiq marked this pull request as ready for review February 23, 2021 09:50
@UsamaSadiq UsamaSadiq requested a review from jmbowman February 23, 2021 09:51
@regisb
Copy link
Copy Markdown
Contributor

regisb commented Feb 23, 2021

How can we achieve this when code-annotations does actually depend on Django? https://github.com/edx/code-annotations/blob/master/code_annotations/find_django.py#L10

@UsamaSadiq
Copy link
Copy Markdown
Member Author

UsamaSadiq commented Feb 23, 2021

How can we achieve this when code-annotations does actually depend on Django? https://github.com/edx/code-annotations/blob/master/code_annotations/find_django.py#L10

@regisb thanks for pointing it out. I didn't look in detail as I thought tests would fail but it turns out I was wrong. @jmbowman since django is required in this package, then should I add the constraint in the base.in file or in the setup.py file instead? so it doesn't affect the other packages/repos which are using this package.

@jmbowman
Copy link
Copy Markdown

My preference would be to make those imports (and the code using them) fail gracefully when Django is not installed, since there shouldn't be any Django models in a package or service that doesn't depend on Django. Then Django would be an optional dependency of code-annotations rather than a required one. The goal here is to avoid introducing Django as a dependency to all our Python repositories that don't already use it (via the dependency of edx-lint on this package).

@UsamaSadiq UsamaSadiq force-pushed the usamasadiq/remove-django-requirement branch 3 times, most recently from d71b78b to dfaecfa Compare May 10, 2021 08:30
@UsamaSadiq
Copy link
Copy Markdown
Member Author

My preference would be to make those imports (and the code using them) fail gracefully when Django is not installed, since there shouldn't be any Django models in a package or service that doesn't depend on Django. Then Django would be an optional dependency of code-annotations rather than a required one. The goal here is to avoid introducing Django as a dependency to all our Python repositories that don't already use it (via the dependency of edx-lint on this package).

Instead of completely removing the django dependency, I've moved it to the dev.in file so it will only be installed as this package's dependency and will not affect other packages which will use code-annotations.

@regisb
Copy link
Copy Markdown
Contributor

regisb commented May 10, 2021

Can I suggest instead adding an extra requirement to setup.py? https://setuptools.readthedocs.io/en/latest/userguide/dependency_management.html?highlight=extras_require#optional-dependencies

extras_require: {"django":  ["Django>=2.2"]}

This allows us to write pip install code-annotations[django], or add code-annotations[django] to any requirements.txt file. This is the "standard" way of doing things; I believe Django uses the same approach to install database-specific requirements.

@UsamaSadiq UsamaSadiq force-pushed the usamasadiq/remove-django-requirement branch from dfaecfa to 7a68dbd Compare May 11, 2021 04:45
@UsamaSadiq
Copy link
Copy Markdown
Member Author

Can I suggest instead adding an extra requirement to setup.py? https://setuptools.readthedocs.io/en/latest/userguide/dependency_management.html?highlight=extras_require#optional-dependencies

extras_require: {"django":  ["Django>=2.2"]}

This allows us to write pip install code-annotations[django], or add code-annotations[django] to any requirements.txt file. This is the "standard" way of doing things; I believe Django uses the same approach to install database-specific requirements.

Added django dependency in extras_require as per suggestion.

@UsamaSadiq UsamaSadiq requested a review from regisb May 11, 2021 06:40
Copy link
Copy Markdown
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review!

Copy link
Copy Markdown

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One optional suggestion for further improvement, otherwise looks good!

Comment thread setup.py
},
include_package_data=True,
install_requires=load_requirements('requirements/base.in'),
extras_require={"django": ["Django>=2.2,<2.3"]},
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly cleaner would be to use something like extras_require={"django": load_requirements('requirements/django.in')}, here, update Makefile to create django.txt from the new input file, and include -r django.txt in requirements/test.in. requirements/django.in would just contain an explanatory comment and the Django>=2.2,<2.3 constraint you have here. This would remove keep Django out of base.in, base.txt, and the core package dependencies while keeping the rest of the requirements files for testing and local development largely unchanged.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, much better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the requirement files as per suggestion.

@UsamaSadiq UsamaSadiq force-pushed the usamasadiq/remove-django-requirement branch from 7a68dbd to 179e164 Compare May 17, 2021 13:13
Comment thread requirements/dev.in Outdated
Comment thread Makefile Outdated
@UsamaSadiq UsamaSadiq force-pushed the usamasadiq/remove-django-requirement branch from 179e164 to 302dcfe Compare May 18, 2021 06:44
@UsamaSadiq UsamaSadiq force-pushed the usamasadiq/remove-django-requirement branch from 302dcfe to 0103924 Compare May 18, 2021 06:46
@UsamaSadiq UsamaSadiq merged commit 8611a28 into master May 18, 2021
@UsamaSadiq UsamaSadiq deleted the usamasadiq/remove-django-requirement branch May 18, 2021 13:42
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 this pull request may close these issues.

4 participants