runserver must be restarted after error in admin.py

[Carl Meyer]

In most cases, runserver is able to pick up changes to files and restart itself automatically. However, if there is an error importing an admin.py file, then the error is fixed and the file saved, A) the dev server does not automatically restart, and B) while the admin will load, the app whose admin.py previously contained an error will be missing from the admin. The dev server must be manually restarted before the app will appear again in the admin.

FWIW, while this seems pretty minor, I'm currently teaching a beginning Django class, and this particular bug caused more confusion during the tutorial (part 2, of course) than any other single issue.

[Karen Tracey]

There is already a ticket open on cases when the dev server does not auto-restart properly: #9589. I'm not entirely sure that the cases you encountered here are covered by the patch on that ticket, but I think it would be better to consolidate discussion of possibly improving the behavior in one ticket so I'm going to close this as a dup of that one. If you have time to check out whether the patch proposed there fixes the cases you have encountered then please report your results in that ticket.

[Carl Meyer]

I've done further work on this and realized that I really did conflate two completely separate issues in filing this bug. Part A - the dev server failing to restart (because the admin.py file that failed to import is not in sys.modules) - is indeed a dupe of #9859 (which IMHO is a strong candidate for wontfix; there's simply no way to fix it without ugly hacks).

However, this bug is worse than #9859 because of part B: the confusing "admin works but my app is missing" behavior that we have currently. Without fixing #9859, the admin-specific behavior here can still be fixed. That behavior is not related to the reloader at all, thus I think it would only confuse matters to discuss it on #9859. Could we reopen this issue to pursue that specific fix (and thus also re-categorize it as an admin bug)?

More on the admin.autodiscover() issue:

It would be sufficient in this case if admin.autodiscover() ran again so it would pick up the (now-fixed) admin.py. Problem is that would reimport all apps' admin.py, causing a bunch of AlreadyRegistered exceptions. In fact, it's specifically prevented from running again by the global LOADING flag which was introduced in r9680 to fix #8245.

It seems to me the #8245 fix was inadequate. jarrow's patch on #8245 (which simply rolls-back changes to the admin-model-registry on a failed admin.py import) both fixes #8245 and does not cause this issue. Patch attached to demonstrate this. For tests, the patch simply includes a modification to the #8245 test; the current bug8245 test specifies bad behavior, because it requires that an admin.py error will only be raised a single time and will then mysteriously disappear until the server is restarted. I modified the test to specify the correct behavior, which is that a bad admin.py will raise its error on each request until it is fixed.

[Carl Meyer]

correct the fix for 8254 to avoid mysteriously-disappearing admin.py

[Carl Meyer]

D'oh, meant #9589 all along there, of course.

[Karen Tracey]

Rather than re-opening this, it might be more useful to start a discussion on the developers list about the problem. Comments in #8245 show that the approach that was checked in was taken after IRC discussion involving at least two committers. For some reason the alternate approach was rejected, and I don't know why that was. I agree that only-pop-up-once-and-then-go-away-until-server-restart exceptions are bad, and it might be worth revisiting. But the ticket tracker is probably not the right place for the discussion (the query on rationale in #8245 went unanswered).

[Carl Meyer]

Ok thanks, I'll bring it up on the developers' list.

[Carl Meyer]

Karen, I'd like to ask again about at least un-closing this issue report. Even if nobody on the dev list has cycles for it right now, this describes a real issue (that is not reported elsewhere), and includes a working patch to fix it. Seems like at the least it could be left unclosed to keep track of it until a dev has time to look at it.

[Karen Tracey]

I don't have time to look into it now, but I agree it's worth looking into so I suppose it might as well be open. If someone can explain the rationale for rejecting the proposed approach in #8245 that would be helpful. Without understanding that I'm reluctant to simply proceed with adopting something that was previously rejected.

[thierry.chich@…]

Hi,

I don't know if it is related, but I have a problem using python manage.py shell that similar to this one:

>>> from reseau.models import VPN
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/home/xx/Programmes/Python/Sitetech3/Sitetech/reseau/models.py", line 48, in <module>
    admin.site.register(TypeSupportDiffusion)
  File "/usr/lib/pymodules/python2.6/django/contrib/admin/sites.py", line 78, in register
    raise AlreadyRegistered('The model %s is already registered' % model.__name__)
AlreadyRegistered: The model TypeSupportDiffusion is already registered

In /home/xx/Programmes/Python/Sitetech3/Sitetech/reseau/models.py, there is a lot of things:

class TypeSupportDiffusion(ModelGenerique):
    typeStructureAdministratives=models.ManyToManyField(TypeStructureAdministrative,blank=True)
admin.site.register(TypeSupportDiffusion)

class VPN is an other model in this file not related directly to TypeSupportDiffusion

Regards,

[Ramiro Morales]

carljm patch updated to r12936

[Ramiro Morales]

I've updated carljm patch to current trunk tip status. In other words:

• It undoes the fix for #8245 done in r9680 and implements the strategy proposed by jarrow in that ticket (Misleading-AlreadyRegistered-03.diff)
• Modifies the bug8245 regression test added in such revision.

Unfortunately no record exist of the IRC discussion Greg Wilson talks about in which he maintained with Malcolm on September '08 that led him to disregard the strategy proposed by jarrow and implement the LOADING global one instead. I've searched my #django-* archives and in ​http://botland.oebfare.com/logger so maybe it was in a private IRC channel?

With this patch I get correctly non-disappearing admin URLs in the testing web server after a 500 error is generated by some e.g. an invalid Python syntax error in a ModelAdmin. More testing and feedback about how it works with other error conditions would be great.

Refs. #8126/r8583, #8245/r9680, #8193/r10088, #10887/r12192.

[Brian Rosner]

(In [12956]) Fixed #11957 -- exceptions in admin.py are no longer hidden after second request

Before you had to restart runserver for the correct exception message to show
up again. Reverts fix in r9680 which has this side-affect.

Thanks to jarrow, carljm and ramiro for their work on the patch and tickets.

[Jacob]

Milestone 1.2 deleted