[Django] #32316: Access __file__ lazily rather than at module level
Django
-------------------------------------+-------------------------------------
Reporter: William | Owner: William Schwartz
Schwartz |
Type: | Status: assigned
Cleanup/optimization |
Component: | Version: master
Uncategorized |
Severity: Normal | Keywords: freezers
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
So-called ''frozen'' Python environments (such as those mentioned in
#30950) that do not set all modules' `__file__` variable, which
[https://docs.python.org/3/reference/import.html#__file__ need not be
defined], cannot even import Django (without some workarounds) because a
small number of Django modules use `__file__` at the module level, in a
class defined at the module level, or in a function that is called
automatically upon import.
Five modules that use `__file__` like this are likely to be imported when
using Django and thereby cause a frozen Python to crash with a `NameError`
or similar exception.
* Importing **`django.forms.renderers`** can be avoided only by avoiding
both forms and the ORM altogether as it's imported from
`django.db.models`.
* Importing **`django.views.debug`** might be avoidable if `DEBUG=False`
or by avoiding all of the views and URLs APIs.
* **`django.utils.version`**'s `get_git_changeset` is called when `django`
is imported in pre-alpha development versions.
* Importing **`django.contrib.auth.password_validation`** is only
avoidable by not using the Auth app.
* **`django.utils.translation.trans_real`** uses `__file__` to find
Django's localization files upon activation; this avoidable only by
setting `USE_I18N=False`. Dealing with `trans_real` is sufficiently thorny
(and, being an English speaker with English-speaking clients, I can avoid
it for now) that I will not address it further here except to say that it
might need to be part of the larger discussion at #30950.
== What this ticket is '''''not'''''
'''''I am not proposing removing use of `__file__` at this time.''''' That
would require a longer discussion of intended semantics such as #30950.
This ticket is ''only'' about removing use of `__file__` at the module (or
class definition) level in Django application code (not test code).
Further '''''I am not proposing banning use of `__file__` at the module
level at this time''''', hence minimal new tests and no update to the
Django coding style documentation. That too would require a longer
conversation.
== Proposed fixes
I have pushed PR GH-XXXX to address the four of those modules other than
`trans_real`. I dealt with each module's use of `__file__` in separate
commits to make them easier to discuss and separate/cherry-pick if needed.
Below I link to the individual commits as I discuss each of the four
modules. These first two are fairly easy, but the second two may require
further consideration.
=== `django.forms.renders`
([https://github.com/wkschwartz/django/commit/54d539c8becbd6d4cfc5bcf01c1be64acd34568d
54d539c])
Remove the undocumented module constant `ROOT` and replace its single use.
=== `django.utils.version`
([https://github.com/wkschwartz/django/commit/f4edc6e412df87e05ed224525ebf1355410fe268
f4edc6e])
Treat the lack of module-global `__file__` the same as a failure of `git
log` by returning `None` from `get_git_changeset`.
=== `django.views.debug`
([https://github.com/wkschwartz/django/commit/07f46b7b26a1a74fa531de834bee240d576b7f85
07f46b7])
The module-level constant `CURRENT_DIR` is used only in the module itself
and is undocumented, so I'm assuming it's an obscure private symbol that
no one will miss. I've replaced it with a module-level private function
`_builtin_template_path` that refactors and centralizes finding built-in
templates for the entire module.
The one tricky part is that #32105 added the `html_template_path` and
`text_template_path` attributes `django.views.debug.ExceptionReporter`. I
didn't want to disturb #32105's goal of making the template paths easily
override-able, so I avoided calling `_builtin_template_path` in the class
definition by making detecting the presence of the attributes in
`__init__` and setting defaults there. Alternatives include making the
attributes properties with setters or cached properties without setters.
=== `django.contrib.auth.password_validation`
([https://github.com/wkschwartz/django/commit/24aa80ba413eeaaec0f3c2cab2f028e0dcbaf99b
24aa80b])
The `CommonPasswordValidator`-class constant `DEFAULT_PASSWORD_LIST_PATH`
is used only in one place, the class's instance constructor. While the
nature of `DEFAULT_PASSWORD_LIST_PATH` is not documented, its existence is
inside the docs for the
[https://docs.djangoproject.com/en/3.1/topics/auth/passwords/#django.contrib.auth.password_validation.CommonPasswordValidator
constructor's signature]. I've changed `DEFAULT_PASSWORD_LIST_PATH` from a
class constant into an instance attribute. Another possibility is making
`DEFAULT_PASSWORD_LIST_PATH` be a `django.utils.functional.classproperty`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32316>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
== Proposed fixes
I have pushed [https://github.com/django/django/pull/13841 PR GH-13841] to
address the four of those modules other than `trans_real`. I dealt with
each module's use of `__file__` in separate commits to make them easier
to discuss and separate/cherry-pick if needed. Below I link to the
individual commits as I discuss each of the four modules. These first two
are fairly easy, but the second two may require further consideration.
=== `django.forms.renders`
([https://github.com/django/django/pull/13841/commits/54d539c8becbd6d4cfc5bcf01c1be64acd34568d
54d539c])
Remove the undocumented module constant `ROOT` and replace its single use.
=== `django.utils.version`
([https://github.com/django/django/pull/13841/commits/f4edc6e412df87e05ed224525ebf1355410fe268
f4edc6e])
Treat the lack of module-global `__file__` the same as a failure of `git
log` by returning `None` from `get_git_changeset`.
=== `django.views.debug`
([https://github.com/django/django/pull/13841/commits/07f46b7b26a1a74fa531de834bee240d576b7f85
07f46b7])
The module-level constant `CURRENT_DIR` is used only in the module itself
and is undocumented, so I'm assuming it's an obscure private symbol that
no one will miss. I've replaced it with a module-level private function
`_builtin_template_path` that refactors and centralizes finding built-in
templates for the entire module.
The one tricky part is that #32105 added the `html_template_path` and
`text_template_path` attributes `django.views.debug.ExceptionReporter`. I
didn't want to disturb #32105's goal of making the template paths easily
override-able, so I avoided calling `_builtin_template_path` in the class
definition by making detecting the presence of the attributes in
`__init__` and setting defaults there. Alternatives include making the
attributes properties with setters or cached properties without setters.
=== `django.contrib.auth.password_validation`
([https://github.com/django/django/pull/13841/commits/24aa80ba413eeaaec0f3c2cab2f028e0dcbaf99b
24aa80b])
The `CommonPasswordValidator`-class constant `DEFAULT_PASSWORD_LIST_PATH`
is used only in one place, the class's instance constructor. While the
nature of `DEFAULT_PASSWORD_LIST_PATH` is not documented, its existence is
inside the docs for the
[https://docs.djangoproject.com/en/3.1/topics/auth/passwords/#django.contrib.auth.password_validation.CommonPasswordValidator
constructor's signature]. I've changed `DEFAULT_PASSWORD_LIST_PATH` from a
class constant into an instance attribute. Another possibility is making
`DEFAULT_PASSWORD_LIST_PATH` be a `django.utils.functional.classproperty`.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tom Forbes):
I think this makes sense as `__file__` is indeed documented as being
optional (which is somewhat surprising). If we do go forward with this,
how do we catch future uses of `__file__` at the module level?
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Tom Forbes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by William Schwartz):
> If we do go forward with this, how do we catch future uses of `__file__`
at the module level?
Great question, and here's my terrible answer: I propose ''not'' to—for
now. This is part of what I meant in my original post about "not proposing
banning use of `__file__` at the module level at this time."
The main reason is that I think testing for module-level use of
`__file__`—as well as all the other subtleties involved in using Python
freezers—will be easiest and most thorough if we now and then (or
regularly on CI...) run the entire Django test suite from inside, say,
[https://pyoxidizer.readthedocs.io/ PyOxidizer] or one of the other full-
featured freezers. But doing that only makes sense in the context of
solving all the impediments to using Django with freezers. As discussed
elsewhere, such as at #30950 and on [https://groups.google.com/g/django-
developers/c/UXsG5xZNRgY django-developers] prior to #32302, that's too
much to solve in one go.
I view this ticket just as a way to nudge Django 3.2 toward more freezer
friendliness before beta testing season. My (small) team then plans to
dogfood the 3.2 betas this spring while developing an app we'll be running
from PyOxidizer, so I'm hoping we'll notice if `__file__` sneaks back in
somewhere that would cause a frozen Python to crash.
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* component: Uncategorized => Core (Other)
* stage: Unreviewed => Accepted
Comment:
Hey William.
OK, let's accept this, but can I ask you to (briefly) write up a "Roadmap
to Freezable Django", perhaps just for the mailing list, rather than a
full DEP (but do use the DEP template if that helps)?
You've obviously got an end goal in mind — and that's probably worthwhile
and something we should have in mind as a target — but these sequence of
little adjustments feel a bit like death-by-a-thousand-cuts (if that makes
sense). No problem with them, but the overview, and the obstacles
currently foreseeable would be good to discuss, and to get wise-folk input
on.
Make sense? 🙂
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by William Schwartz):
Yes, I will absolute write a broader memo to the mailing list. It is not
my intention to wear down y'all maintainers!
My plan all along has been to get this and a couple other tickets (#32302
and #32317 in particular) taken care of before the 3.2 feature freeze on
the 14th, experiment with the results for awhile and consult my
colleagues, and then write up something more big-picture for the mailing
list. (I'll check out the DEP template—thanks for the idea.) While I have
an idea of what I want to propose, getting some practical experience with
a frozen, Django project seems prudent before moving beyond minor
cleanups. Besides, my time is extremely constrained at the moment, so
it'll be several weeks before I can write up anything anyway.
Thanks for continuing to work with me on these incremental changes in the
meantime!
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Patch needs minor adjustments and then it should be good to go.
Should wait until after #32355 so as to be able to use
`functools.cached_property`
(assuming we'll deprecate Django's own once 3.8 is the minimum supported
version.)
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:10>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:11>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"7248afe12f40361870388ecdd7e0038eb0d58e47" 7248afe1]:
{{{
#!CommitTicketReference repository=""
revision="7248afe12f40361870388ecdd7e0038eb0d58e47"
Refs #32105 -- Moved ExceptionReporter template paths to properties.
Refs #32316.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:13>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: freezers | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"a118564ae167fa80c2628b1d22292794af4c0341" a118564a]:
{{{
#!CommitTicketReference repository=""
revision="a118564ae167fa80c2628b1d22292794af4c0341"
[3.2.x] Refs #32105 -- Moved ExceptionReporter template paths to
properties.
Refs #32316.
Backport of 7248afe12f40361870388ecdd7e0038eb0d58e47 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:14>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:15>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Component: Core (Other) | Version: dev
Keywords: freezers | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"9ee693bd6cf4074f04ec51c6f3cfe87cad392f12" 9ee693bd]:
{{{
#!CommitTicketReference repository=""
revision="9ee693bd6cf4074f04ec51c6f3cfe87cad392f12"
Fixed #32316 -- Deferred accessing __file__.
Deferred accessing the module-global variable __file__ because the
Python import API does not guarantee it always exists—in particular, it
does not exist in certain "frozen" environments. The following changes
advanced this goal.
Thanks to Carlton Gibson, Tom Forbes, Mariusz Felisiak, and Shreyas
Ravi for review and feedback.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:16>
Django
-------------------------------------+-------------------------------------
Type: | Schwartz
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: dev
Severity: Normal | Resolution: fixed
Keywords: freezers | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by William Schwartz):
I'm cleaning up random files on my desktop and I came across these notes I
left for myself while working on this bug. I'm leaving them here for
posterity.
= Appendix: Other modules that use `__file__`.
From `master` at b41d38ae26b1da9519a6cd765bc2f2ce7d355007:
{{{#!console
$ git grep --files-with-matches -e "__file__" b41d38a -- django | wc -l
b41d38a:django/apps/config.py
b41d38a:django/conf/__init__.py
b41d38a:django/conf/project_template/project_name/settings.py-tpl
b41d38a:django/contrib/auth/password_validation.py
b41d38a:django/core/management/commands/makemessages.py
b41d38a:django/db/migrations/loader.py
b41d38a:django/db/migrations/questioner.py
b41d38a:django/forms/renderers.py
b41d38a:django/utils/autoreload.py
b41d38a:django/utils/module_loading.py
b41d38a:django/utils/translation/trans_real.py
b41d38a:django/utils/version.py
b41d38a:django/views/debug.py
}}}
The remainder of the files listed in the `git grep` output above do
**not** need to be addressed in this ticket at this time. This section
explains why.
The following modules access `__file__` only through proper `getattr` or
`hasattr` guards:
* `django.apps.config` to compute `AppConfig.path` from its `module`
* `django.db.migrations.loader` (see #32302)
* `django.db.migrations.questioner` in `MigrationQuestioner.ask_initial`
* `django.utils.module_loading` in `module_dir`
`django.utils.autoreload` doesn't access `__file__` at the module level.
`iter_modules_and_files` has a proper `hasattr` guard but `is_django_path`
would need different semantics. #32314 addresses `get_child_arguments`.
`django.conf.LazySettings.PASSWORD_RESET_TIMEOUT_DAYS` uses `__file__`
only to avoid warning about this deprecated setting. Frozen applications
can avoid crashes simply by upgrading away from
`PASSWORD_RESET_TIMEOUT_DAYS`.
`django/conf/project_template/project_name/settings.py-tpl` is the
template settings module. Django projects are supposed to edit this
anyway.
Finally the `makemessages` command has no use for projects with
`USE_I18N=False`, which is necessary for frozen projects to work around
the issue with `trans_real` described above.
--
Ticket URL: <https://code.djangoproject.com/ticket/32316#comment:17>