{{{
[autoreload of db.models failed: Traceback (most recent call last):
File "/pyenvs/p3/lib/python3.4/site-
packages/IPython/extensions/autoreload.py", line 247, in check
superreload(m, reload, self.old_objects)
RuntimeError: Conflicting 'language' models in application 'db': <class
'db.models.Language'> and <class 'db.models.Language'>.
}}}
The problem is - when reloading a module - it has absolutely the same path
and it cannot be changed to prevent a confusion with different modules
(what this exception tries to do)
I tried this for example:
{{{
if model_name in app_models and str(app_models[model_name]) != str(model):
...
}}}
Found it here:
https://github.com/django/django/commit/aff57793b46e108c6e48d5ce3b889bf90b513df9
#diff-e0a696d19bf764562a439d3080a30fda
Pull request: https://github.com/django/django/pull/3310
--
Ticket URL: <https://code.djangoproject.com/ticket/23621>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Hi,
I can't seem to be able to reproduce this issue.
Could you describe what you're doing exactly to trigger this error.
I'm not at all familiar with ipython, but here's what I've tried:
* Created and activated a virtualenv
* `pip install ipython`
* Run `./manage.py` inside a test project.
* Typed `%load_ext autoreload` and then `%autoreload` (as per
http://ipython.org/ipython-doc/stable/config/extensions/autoreload.html).
When doing so, I don't get any error message. Am I missing something?
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:1>
Comment (by apollo13):
I think it's dangerous to even try supporting module reloads. While this
change might work for this special issue, it can't be proven that it will
work in every case, leading to hard to debug issues if this ever fails.
Considering that, it leaves me at -0 to -1.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:2>
Comment (by bmispelon):
For reference, I finally managed to reproduce this by adding the following
steps:
* Change a `models.py` file from an app that's in INSTALLED_APPS
* Run `%autoreload`
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:3>
Comment (by collinanderson):
I agree that this sounds dangerous. It doesn't seem like something that
would work reliably.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:4>
Comment (by carljm):
I don't like vague FUDy arguments against fixing a regression. Reloading a
models module worked in previous versions of Django (to the same extent
that it ever works in Python), and I think if we are going to explicitly
disallow it starting with Django 1.7 we should at the very least be able
to give sound and specific reasons.
The general problem with module reloading in Python is that other modules
may (and probably do) still have references to classes or functions from
the old, not-reloaded version of the module. And that is quite likely with
Django - for instance if there are FK references to or from the reloaded
model, there will be related-objects and `_meta` modules still holding
references to the old version of the model, and that could cause all kinds
of strange behavior.
But there are simple cases where it _doesn't_ break and makes life much
more convenient, i.e. when doing rapid iteration and interactive shell
testing of a single module, without involving any other modules. I'm
guessing this is the OP's use case. So it seems to me that Django could
continue to "support" module reloading to the same extent Python does:
it's possible, but in non-triviel cases it's likely to break, and when it
breaks you get to keep all the pieces.
On the other hand, it won't bother me too much if we just say "sorry,
module reloading is too likely to cause obscure bugs, just don't do it".
So I guess I'm +0.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:5>
Comment (by collinanderson):
Maybe we could change it to a warning?
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:6>
Comment (by aaugustin):
I don't have the time to look at this right now, but I'd like to think a
bit about the consequences before making a decision (if possible).
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:7>
Comment (by dfunckt):
FWIW, this also happens in regular shell, using the builtin `reload`:
{{{
>>> from myproject.myapp import models as mymodels
>>> mymodels.Person.objects.get(pk=5)
<Person 0x123456789>
# Edit myproject/myapp/models.py
>>> reload(mymodels)
Exception...
}}}
I believe this regression is unfortunate, since it is quite useful to be
able to play-edit-play with your app's models, but I can understand the
complexity of trying to fix it given the App refactoring in 1.7.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:8>
Comment (by aaugustin):
Going from "fails silently with subtle and inconsistent effects" to "fails
loudly with an exception" doesn't count as a regression in my book :-)
When you used `reload()` in Django 1.6, there was no guarantee that you
ended up in a consistent state. Specifically, accessing related model
instances could return an obsolete copy of the model class. And I'm
wondering if the change proposed here couldn't make that kind of subtle
bugs possible again.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:9>
Comment (by dfunckt):
Agreed -- though most of the time, and for the simpler cases it would work
just fine. In the face of App refactor and its benefits though, I find
this behaviour acceptable. I'd be more inclined for a note in
documentation perhaps with some helpful explanation.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:10>
Comment (by dfunckt):
That said, I am actually supposing that it is a major undertaking to make
reloading work -- perhaps I am mistaken -- and I wouldn't want to dismiss
the original report.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:11>
Comment (by carljm):
I guess the question is whether the potential brokenness of module
reloading is Django-specific, or inherent to module reloading in Python.
If the latter, it's more like we're saying "here's a feature of Python
that has some gotchas, and we think the gotchas are bad enough that the
feature shouldn't be in Python at all, so we're going to make it
impossible to use it with Django."
Module reloading _is_ inherently problematic in Python anytime one module
holds references to anything from another module. But it's perhaps a
little worse with Django, since models are highly likely to hold
references to one another?
If this were hard to implement, I think it'd be an easy choice not to. But
in fact, it's just slightly relaxing a one-liner check.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:12>
Comment (by carljm):
Replying to [comment:9 aaugustin]:
> When you used `reload()` in Django 1.6, there was no guarantee that you
ended up in a consistent state. Specifically, accessing related model
instances could return an obsolete copy of the model class. And I'm
wondering if the change proposed here couldn't make that kind of subtle
bugs possible again.
To be clear, the change proposed here would absolutely make that kind of
subtle bug possible again. That type of bug is possible anytime you reload
modules in Python, with or without Django.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:13>
Comment (by aaugustin):
Considering what Carl just said, I think we should raise a
`RuntimeWarning` (which I believe is loud by default) instead of an
exception in the reloading case.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:14>
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:15>
Comment (by dfunckt):
Ah, didn't see there was a PR already (3310) -- mine (3399) fixes this as
@aaugustin suggested, that is by raising a warning instead of an
exception. Just close it if you see fit.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:16>
* has_patch: 0 => 1
* component: Uncategorized => Core (Other)
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:17>
Comment (by loic):
> Considering what Carl just said, I think we should raise a
`RuntimeWarning` (which I believe is loud by default) instead of an
exception in the reloading case.
I checked https://github.com/django/django/pull/3399 but my understanding
of "in the reloading case" would imply something more like (basically a
mix of both PRs):
{{{
message = "Conflicting '%s' models in application '%s': %s and %s." % (
model_name, app_label, app_models[model_name], model)
if (model.__name__ == app_models[model_name].__name__ and
model.__module__ == app_models[model_name].__module__):
warnings.warn(message, RuntimeWarning, stacklevel=2)
else:
raise RuntimeError(message)
}}}
@aaugustin is that what you meant?
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:18>
* cc: loic (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:19>
Comment (by aaugustin):
Yes, Loic's proposal in comment 18 implements the solution I was
attempting to describe.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:20>
Comment (by dfunckt):
Regarding PR 3399, I wasn't sure how to differentiate between calls to
{{{register_model}}} because of module reloading or because of normal
model loading. I was about to ask for guidance in the PR, but then saw the
existing PR and forgot about it. Excuse me for the confusion.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:21>
Comment (by loic):
Turned the snippet above into a PR:
https://github.com/django/django/pull/3410.
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:22>
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:23>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"8c4ca16c65174db63471f12b005f5423b61de073"]:
{{{
#!CommitTicketReference repository=""
revision="8c4ca16c65174db63471f12b005f5423b61de073"
Fixed #23621 -- Warn for duplicate models when a module is reloaded.
Previously a RuntimeError was raised every time two models clashed
in the app registry. This prevented reloading a module in a REPL;
while it's not recommended to do so, we decided not to forbid this
use-case by turning the error into a warning.
Thanks @dfunckt and Sergey Pashinin for the initial patches.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:24>
Comment (by Loic Bistuer <loic.bistuer@…>):
In [changeset:"b62f72498af8a8cb4ddb3000421c5642bda57761"]:
{{{
#!CommitTicketReference repository=""
revision="b62f72498af8a8cb4ddb3000421c5642bda57761"
Improved warning message when reloading models. Refs #23621.
Thanks dfunckt and Tim Graham.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:25>
Comment (by Loic Bistuer <loic.bistuer@…>):
In [changeset:"7fa6781f818deea61cef4c13b139fa9586a6ca7a"]:
{{{
#!CommitTicketReference repository=""
revision="7fa6781f818deea61cef4c13b139fa9586a6ca7a"
[1.7.x] Fixed #23621 -- Warn for duplicate models when a module is
reloaded.
Previously a RuntimeError was raised every time two models clashed
in the app registry. This prevented reloading a module in a REPL;
while it's not recommended to do so, we decided not to forbid this
use-case by turning the error into a warning.
Thanks dfunckt and Sergey Pashinin for the initial patches.
Backport of 8c4ca16c65 and b62f72498a from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:26>
Comment (by Tim Graham <timograham@…>):
In [changeset:"01b4a13db4d544e71d91bc5f3e9d6e9a44d3ed75"]:
{{{
#!CommitTicketReference repository=""
revision="01b4a13db4d544e71d91bc5f3e9d6e9a44d3ed75"
Forwardported release note for refs #23621.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:27>
Comment (by Tim Graham <timograham@…>):
In [changeset:"6149a0ab1804e238551424585a346166186fad6f"]:
{{{
#!CommitTicketReference repository=""
revision="6149a0ab1804e238551424585a346166186fad6f"
[1.7.x] Edited release note for refs #23621.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:28>
* status: closed => new
* resolution: fixed =>
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:29>
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:30>
Comment (by Tim Graham <timograham@…>):
In [changeset:"aabb58428beae0bd34f32e5d620a82486b670499" aabb584]:
{{{
#!CommitTicketReference repository=""
revision="aabb58428beae0bd34f32e5d620a82486b670499"
Refs #23621 -- Fixed warning message when reloading models.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:31>
Comment (by Tim Graham <timograham@…>):
In [changeset:"42aa919de9ef9d039a72e468bdb4e868daa32e5c" 42aa919]:
{{{
#!CommitTicketReference repository=""
revision="42aa919de9ef9d039a72e468bdb4e868daa32e5c"
[1.8.x] Refs #23621 -- Fixed warning message when reloading models.
Backport of aabb58428beae0bd34f32e5d620a82486b670499 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:33>
Comment (by Tim Graham <timograham@…>):
In [changeset:"9bd3a2325e22bdc034ec2646afc412a1d3f16a71" 9bd3a23]:
{{{
#!CommitTicketReference repository=""
revision="9bd3a2325e22bdc034ec2646afc412a1d3f16a71"
[1.7.x] Refs #23621 -- Fixed warning message when reloading models.
Backport of aabb58428beae0bd34f32e5d620a82486b670499 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23621#comment:32>