Module autoreloading doesn't work due to some checks while loading an app

[Sergey Pashinin]

When in IPython shell: ./manage.py shell module autoreloading doesn't work and gives this error:

[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

[Baptiste Mispelon]

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.

[Florian Apolloner]

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.

[Baptiste Mispelon]

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

[Collin Anderson]

I agree that this sounds dangerous. It doesn't seem like something that would work reliably.

[Carl Meyer]

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.

[Collin Anderson]

Maybe we could change it to a warning if it's the same path?

[Aymeric Augustin]

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

[Akis Kesoglou]

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.

[Aymeric Augustin]

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.

[Akis Kesoglou]

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.

[Akis Kesoglou]

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.

[Carl Meyer]

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.

[Carl Meyer]

Replying to 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.

[Aymeric Augustin]

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.

[Akis Kesoglou]

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.

[Loic Bistuer]

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?

[Aymeric Augustin]

Yes, Loic's proposal in comment 18 implements the solution I was attempting to describe.

[Akis Kesoglou]

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.

[Loic Bistuer]

Turned the snippet above into a PR: ​https://github.com/django/django/pull/3410.

[Loic Bistuer <loic.bistuer@…>]

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

[Loic Bistuer <loic.bistuer@…>]

In b62f72498af8a8cb4ddb3000421c5642bda57761:

Improved warning message when reloading models. Refs #23621.

Thanks dfunckt and Tim Graham.

[Loic Bistuer <loic.bistuer@…>]

In 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

[Tim Graham <timograham@…>]

In 01b4a13db4d544e71d91bc5f3e9d6e9a44d3ed75:

Forwardported release note for refs #23621.

[Tim Graham <timograham@…>]

In 6149a0ab1804e238551424585a346166186fad6f:

[1.7.x] Edited release note for refs #23621.

[Tim Graham <timograham@…>]

In aabb584:

Refs #23621 -- Fixed warning message when reloading models.

[Tim Graham <timograham@…>]

In 9bd3a23:

[1.7.x] Refs #23621 -- Fixed warning message when reloading models.

Backport of aabb58428beae0bd34f32e5d620a82486b670499 from master

[Tim Graham <timograham@…>]

In 42aa919:

[1.8.x] Refs #23621 -- Fixed warning message when reloading models.

Backport of aabb58428beae0bd34f32e5d620a82486b670499 from master