Opened 6 years ago

Closed 6 years ago

Last modified 5 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 Changed 6 years ago by 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:

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

Thanks.

comment:2 Changed 6 years ago by 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.

comment:3 Changed 6 years ago by 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

comment:4 Changed 6 years ago by Collin Anderson

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

comment:5 Changed 6 years ago by 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.

comment:6 Changed 6 years ago by Collin Anderson

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

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

comment:7 Changed 6 years ago by 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).

comment:8 Changed 6 years ago by 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.

comment:9 Changed 6 years ago by 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.

comment:10 Changed 6 years ago by 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.

comment:11 Changed 6 years ago by 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.

comment:12 Changed 6 years ago by 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.

comment:13 in reply to:  9 Changed 6 years ago by 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.

comment:14 Changed 6 years ago by 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.

comment:15 Changed 6 years ago by Collin Anderson

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:16 Changed 6 years ago by 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.

comment:17 Changed 6 years ago by Tim Graham

Component: UncategorizedCore (Other)
Has patch: set

comment:18 Changed 6 years ago by 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?

comment:19 Changed 6 years ago by Loic Bistuer

Cc: Loic Bistuer added

comment:20 Changed 6 years ago by Aymeric Augustin

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

comment:21 Changed 6 years ago by 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.

comment:22 Changed 6 years ago by Loic Bistuer

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

comment:23 Changed 6 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:24 Changed 6 years ago by Loic Bistuer <loic.bistuer@…>

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 Changed 6 years ago by Loic Bistuer <loic.bistuer@…>

In b62f72498af8a8cb4ddb3000421c5642bda57761:

Improved warning message when reloading models. Refs #23621.

Thanks dfunckt and Tim Graham.

comment:26 Changed 6 years ago by 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

comment:27 Changed 6 years ago by Tim Graham <timograham@…>

In 01b4a13db4d544e71d91bc5f3e9d6e9a44d3ed75:

Forwardported release note for refs #23621.

comment:28 Changed 6 years ago by Tim Graham <timograham@…>

In 6149a0ab1804e238551424585a346166186fad6f:

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

comment:29 Changed 6 years ago by None

Resolution: fixed
Status: closednew

comment:30 Changed 6 years ago by None

Resolution: fixed
Status: newclosed

comment:31 Changed 5 years ago by Tim Graham <timograham@…>

In aabb584:

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

comment:32 Changed 5 years ago by Tim Graham <timograham@…>

In 9bd3a23:

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

Backport of aabb58428beae0bd34f32e5d620a82486b670499 from master

comment:33 Changed 5 years ago by Tim Graham <timograham@…>

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