Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#23621 closed Bug (fixed)

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

Reported by: Sergey Pashinin Owned by: nobody
Component: Core (Other) Version: 1.7
Severity: Normal Keywords:
Cc: Loic Bistuer Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (33)

comment:1 by Baptiste Mispelon, 10 years ago

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:

When doing so, I don't get any error message. Am I missing something?

Thanks.

comment:2 by Florian Apolloner, 10 years ago

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.

comment:3 by Baptiste Mispelon, 10 years ago

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

comment:4 by Collin Anderson, 10 years ago

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

comment:5 by Carl Meyer, 10 years ago

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.

comment:6 by Collin Anderson, 10 years ago

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

Last edited 10 years ago by Collin Anderson (previous) (diff)

comment:7 by Aymeric Augustin, 10 years ago

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

comment:8 by Akis Kesoglou, 10 years ago

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.

comment:9 by Aymeric Augustin, 10 years ago

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.

comment:10 by Akis Kesoglou, 10 years ago

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.

comment:11 by Akis Kesoglou, 10 years ago

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.

comment:12 by Carl Meyer, 10 years ago

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.

in reply to:  9 comment:13 by Carl Meyer, 10 years ago

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.

comment:14 by Aymeric Augustin, 10 years ago

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.

comment:15 by Collin Anderson, 10 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:16 by Akis Kesoglou, 10 years ago

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.

comment:17 by Tim Graham, 10 years ago

Component: UncategorizedCore (Other)
Has patch: set

comment:18 by Loic Bistuer, 10 years ago

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?

comment:19 by Loic Bistuer, 10 years ago

Cc: Loic Bistuer added

comment:20 by Aymeric Augustin, 10 years ago

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

comment:21 by Akis Kesoglou, 10 years ago

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.

comment:22 by Loic Bistuer, 10 years ago

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

comment:23 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:24 by Loic Bistuer <loic.bistuer@…>, 10 years ago

Resolution: fixed
Status: newclosed

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.

comment:25 by Loic Bistuer <loic.bistuer@…>, 10 years ago

In b62f72498af8a8cb4ddb3000421c5642bda57761:

Improved warning message when reloading models. Refs #23621.

Thanks dfunckt and Tim Graham.

comment:26 by Loic Bistuer <loic.bistuer@…>, 10 years ago

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

comment:27 by Tim Graham <timograham@…>, 10 years ago

In 01b4a13db4d544e71d91bc5f3e9d6e9a44d3ed75:

Forwardported release note for refs #23621.

comment:28 by Tim Graham <timograham@…>, 10 years ago

In 6149a0ab1804e238551424585a346166186fad6f:

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

comment:29 by None, 10 years ago

Resolution: fixed
Status: closednew

comment:30 by None, 10 years ago

Resolution: fixed
Status: newclosed

comment:31 by Tim Graham <timograham@…>, 9 years ago

In aabb584:

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

comment:32 by Tim Graham <timograham@…>, 9 years ago

In 9bd3a23:

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

Backport of aabb58428beae0bd34f32e5d620a82486b670499 from master

comment:33 by Tim Graham <timograham@…>, 9 years ago

In 42aa919:

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

Backport of aabb58428beae0bd34f32e5d620a82486b670499 from master

Note: See TracTickets for help on using tickets.
Back to Top