Unnecessary imports lead to ImportError

[Klaas van Schelven]

django/utils/translation/trans_real.py contains

         for appname in settings.INSTALLED_APPS:
            app = import_module(appname)
            apppath = os.path.join(os.path.dirname(app.__file__), 'locale')

As you can see, the only reason apps are imported here is to determine their location so a locale path can be tucked on. However, importing apps comes at a cost. There are cases in which this import leads to a circular reference, leading to the inability of the application to load.

There may be more locations where this particular loop over INSTALLED_APPS leads to problems, though I have not encountered them.

On a somewhat related note, I don't think Django provides us with an easy way to loop over the installed apps. We have models.get_apps, but that doesn't work for modelless apps. This might lead to issues such as the one I reported here:
​https://github.com/jezdez/django-staticfiles/issues#issue/2

I'll try to reproduce the negative consequences (the ImportError) in a few moments and post the results here. I also have some kind of patch for this running in production, but I can imagine it's not really core-proof. Evaluation is appreciated.

[Klaas van Schelven]

I'm not able to reproduce the error in a simple example. It does occur in my full project. However, I cannot simply put a full proprietary project online. All I can do at this point is hope someone else has a similar experience and a simple example.

I'll also produce the "works for me" patch mentioned above.

[Klaas van Schelven]

This is my 'works for me' solution, I can imagine it has some hairy consequences (Jython?)

def get_apppath(appname):
    def get(base, parts):
        l0 = [base] + parts + ["__init__.py"]
        l1 = [base] + parts + ["__init__.pyc"]
        if not (os.path.isfile(os.path.join(*l0)) or os.path.isfile(os.path.join(*l1))):
            return None
        return os.path.join(l0[:-1])

    import sys
    import os
    parts = appname.split(".")
    for base in sys.path:
        result = get(base, parts)
        if not get(base, parts) is None:
            return os.path.join(base, *parts)
    raise Exception("No such app: %s" % appname)

Combined, obviously, with a call to get_apppath instead of import_module in trans_real.py

[jeff@…]

Suggested, untested fix

[jeff@…]

What about the attached patch? It may not totally replicate the logic in your solution above, but appears to do the same thing that Django has been doing without actually importing the app.

[Klaas van Schelven]

The suggested fix looks a lot better to me than what I came up with. I think a better way to phrase it would be "the logic in my solution above does not totally replicate the attached patch". Better to use a python built in than to "roll your own"

[jeff@…]

Tested, working fix - derp, other one was always trying to find a module called "app"

[Russell Keith-Magee]

Accepting this on the basis that it's yet another of those nebulous tickets that falls in the ​"Django should report errors better" bucket.

The pkgutils-based "look for a package rather than importing it" solution looks promising to me. Marking as part of the i18n framework because the specific error handling case stems from there.

[Klaas van Schelven]

Thanks for accepting it. Please do note that this is not just a matter of "error reporting" but will actually cause ImportErrors in some cases.

What needs to be done for this to be ready for checkin?

[Russell Keith-Magee]

I think you'll find that an ImportError is an Error. Hence, this is about Error reporting :-)

To get this RFC, someone other than the patch author needs to audit the patch and say that it's correct. @vanschelven; given that you didn't write the patch, you can do that job.

In addition, since the the patch doesn't contain a programatic test case, it will definitely need a clear set of instructions for how to reproduce the problem. From a quick scan of the ticket, this hasn't been established yet. Broadly, I can see how the problem might occur, but it isn't immediately obvious what I'd need to do in order to validate for myself that this patch does what it claims.

[Klaas van Schelven]

Russell: by that logic every Error is a "Error reporting" problem. :-)

Anyway, I've been trying for much too long again to recreate a clean environment in which this bug actually causes harm, to no avail.

I can, however, confirm the following:

1. In my production system, without patch, Django crashes with an ImportError as described
2. With Jeff's patch the project loads
3. Adding some print statements/ logging seems to indicate that indeed the right paths are considered.

As I said, I've started chopping parts off my project to get to try to isolate an exportable, isolated, problem, but not getting very far. I'm posting the Traceback (after adding a "raise" to django.core.management.base as decribed in #11667) here, in case this may be of use to someone:

Traceback (most recent call last):                                     
  File "manage.py", line 11, in <module>                               
    execute_manager(settings)                                          
  File "/home/klaas/dev/importerror/src/django-tip/django/core/management/__init__.py", line 438, in execute_manager
    utility.execute()                                                                                               
  File "/home/klaas/dev/importerror/src/django-tip/django/core/management/__init__.py", line 379, in execute        
    self.fetch_command(subcommand).run_from_argv(self.argv)                                                         
  File "/home/klaas/dev/importerror/src/django-tip/django/core/management/base.py", line 191, in run_from_argv      
    self.execute(*args, **options.__dict__)                                                                         
  File "/home/klaas/dev/importerror/src/django-tip/django/core/management/base.py", line 209, in execute            
    translation.activate('en-us')                                                                                   
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/translation/__init__.py", line 81, in activate      
    return _trans.activate(language)                                                                                
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/translation/trans_real.py", line 200, in activate   
    _active[currentThread()] = translation(language)                                                                
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/translation/trans_real.py", line 183, in translation
    default_translation = _fetch(settings.LANGUAGE_CODE)
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/translation/trans_real.py", line 166, in _fetch
    app = import_module(appname)
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/importlib.py", line 35, in import_module
    __import__(name)
  File "/home/klaas/dev/importerror/src/online-apps-tip/numerology/__init__.py", line 1, in <module>
    from models import HasNumberFormat
  File "/home/klaas/dev/importerror/src/online-apps-tip/numerology/models.py", line 4, in <module>
    class HasTypedNumberFormat(models.Model):
  File "/home/klaas/dev/importerror/src/online-apps-tip/numerology/models.py", line 5, in HasTypedNumberFormat
    number = models.IntegerField(_("Number"))
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/translation/__init__.py", line 62, in ugettext
    return _trans.ugettext(message)
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/translation/trans_real.py", line 283, in ugettext
    return do_translate(message, 'ugettext')
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/translation/trans_real.py", line 273, in do_translate
    _default = translation(settings.LANGUAGE_CODE)
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/translation/trans_real.py", line 183, in translation
    default_translation = _fetch(settings.LANGUAGE_CODE)
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/translation/trans_real.py", line 166, in _fetch
    app = import_module(appname)
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/importlib.py", line 35, in import_module
    __import__(name)
  File "/home/klaas/dev/importerror/usesadmin/__init__.py", line 1, in <module>
    import admin
  File "/home/klaas/dev/importerror/usesadmin/admin.py", line 6, in <module>
    admin.site.register(Something, SomethingAdmin)
  File "/home/klaas/dev/importerror/src/django-tip/django/contrib/admin/sites.py", line 90, in register
    validate(admin_class, model)
  File "/home/klaas/dev/importerror/src/django-tip/django/contrib/admin/validation.py", line 21, in validate
    models.get_apps()
  File "/home/klaas/dev/importerror/src/django-tip/django/db/models/loading.py", line 115, in get_apps
    self._populate()
  File "/home/klaas/dev/importerror/src/django-tip/django/db/models/loading.py", line 61, in _populate
    self.load_app(app_name, True)
  File "/home/klaas/dev/importerror/src/django-tip/django/db/models/loading.py", line 76, in load_app
    app_module = import_module(app_name)
  File "/home/klaas/dev/importerror/src/django-tip/django/utils/importlib.py", line 35, in import_module
    __import__(name)
  File "/home/klaas/dev/importerror/client/__init__.py", line 1, in <module>
    from numerology.models import HasTypedNumberFormat
ImportError: cannot import name HasTypedNumberFormat

As far as I'm concerned, this is ready for checkin now.

[Jannis Leidel]

First, pkgutil.get_loader isn't included in Python 2.4 so this would definitely need some backporting.

But even that, I'm not even sure get_loader is the correct way to get an object, it's returning an importer after all, not an app.

Given the fact we are most likely to get a saner app loading in the next release cycle, I think this should wait for the cache being able to correctly iterate over installed apps.

[Klaas van Schelven]

Replying to jezdez:

First, pkgutil.get_loader isn't included in Python 2.4 so this would definitely need some backporting.

That's a valid point. I'm tempted to look into a solution, but only if it has some chance to actually make it into the code, given your next remark.

But even that, I'm not even sure get_loader is the correct way to get an object, it's returning an importer after all, not an app.

Given the fact we are most likely to get a saner app loading in the next release cycle, I think this should wait for the cache being able to correctly iterate over installed apps.

I'm not sure this will lead to a workable solution. As you may imagine the translation machinery needs to be available before all apps are loaded, since some apps may actually contain translations. If you're intending to use the cache that seems to indicate you still want to preserve the behavior of "importing stuff to find out where it is located". That just seems like a objectively BadThing to me.

[Jannis Leidel]

Replying to vanschelven:

I'm not sure this will lead to a workable solution. As you may imagine the translation machinery needs to be available before all apps are loaded, since some apps may actually contain translations. If you're intending to use the cache that seems to indicate you still want to preserve the behavior of "importing stuff to find out where it is located". That just seems like a objectively BadThing to me.

While you keep saying it's bad, you still haven't shown a specific case of bad behavior. Hence I see no reason to stop using the well defined pattern of importing apps to figure out where they are located. Catching those ImportErrors and showing a nice message telling the user why something didn't work, is certainly a good idea.

[Klaas van Schelven]

Give me some time and I may invest some more into finding an isolated demonstration of the problem. In the meantime, the given patch is not stable:

  File "/home/klaas/dev/online/scheffer/django/utils/translation/trans_real.py", line 161, in _fetch                                                           
    apppath = os.path.join(app.filename, 'locale')                                                                                                             
AttributeError: 'NoneType' object has no attribute 'filename'    

under certain circumstances.

[anonymous]

Replying to vanschelven:
Well, that error would occur when app returned None, meaning pkgutils couldn't find the package specified. It'd be interesting to know why pkgutils works around the errors that exist when your import directly, but doesn't seem to catch all cases.

Do you have other core changes that might be poisoning the app loading process here? This issue seems pretty isolated and if both import and pkgutils fail occasionally, I'm curious if it may be caused by changes specific to your environment.

[Jannis Leidel]

We could probably take a peek in what we do here: source:django/trunk/django/core/management/__init__.py#L31

Maby even abstract it.

[Jannis Leidel]

Um, I meant ​http://code.djangoproject.com/browser/django/trunk/django/core/management/__init__.py?rev=13489#L31

[Ramiro Morales]

Given the fact that the reported error condition can't be reproduced in a reduced test case, it would be nice if the OP describes the setup used in the production deployment where the problem happens.

[James Addison]

I am 99% certain that this issue manifests itself when using django-haystack.

[Ramiro Morales]

Closing needsinfo in wait of a description of the pre-conditions that allow reproduction of the issue.

[Klaas van Schelven]

Just noting that no Django 1.4 jezdez' objection that pkgutil.get_loader isn't available in Python 2.4 is no longer relevant since that version of Python is not supported for 1.4 and up.

If I'll ever get to reproducing the bug in a clean environment I'll post that here as well.

[Klaas van Schelven]

Ok, I'm reopening this as I finally acquired the understanding of what's really going wrong and managed to create an "easily" reproducible scenario (not so much test; but that's hard in the case of the current global app-loading anyway).

Create 2 apps.

# defining_app/__init__.py
from django.utils.translation import ugettext

class TranslatedClass(object):
    value = ugettext("Field")

# using_app/__init__.py
from defining_app import TranslatedClass

Add them to INSTALLED_APPS as such (the order is important):

# settings.py

INSTALLED_APPS = (
    # ...
    'using_app',
    'defining_app',
)

This will fail in Django up to 1.4; but succeed with the patch provided by Jeff.

Why?

• Pretty much any Django command calls activate() somewhere. This will trigger the reverse loop over the INSTALLED_APPS. and will start importing apps (​https://github.com/django/django/blob/1.4/django/utils/translation/trans_real.py#L159)
• defining_app is at the bottom of the list; it will be imported
• When evaluating 'value = uggettext("Field")' we go into another reverse loop over INSTALLED_APPS as dictated by trans_real.py
• By now 'defining_app' is in sys.modules, so that import statement doesn't trigger further evaluations
• Next up in the loop: using_app.
• using_app has an "from defining_app import TranslatedClass" statement; however, since we're still somewhere on the stack of getting 'value', TranslatedClass is not available yet.
• ImportError.

In other words, the current implementation of uggettext sneaks in a dependency/call to all installed apps' init.py files. Additionally, because the import happens half-way in a class definition (a rather usual pattern in Django) the class definition cannot be finished in time for the using_module's code to access it.

Why the solution works is simple: not importing modules at (for the user of Django) seemingly random moments will remove the dependency, and allow for TranslatedClass to be added to the module's dictionary of available variables.

I hope this reopens the issue for renewed attention. I _do_ consider this a serious issue, even though apparently not many people have found their way to this page or found a way to isolate the problem. I'd consider doing unasked-for imports something we need to do as little as possible (a.k.a. bad engineering). Especially the fact that uggettext() & friends may trigger this makes this a hairy bug: uggettext is often used in the middle of a module or class definition.

As to the solution: I'm not sure about the corner cases but willing to help out. I'll start by providing a patch that deals with the known return values for get_loader.

[Klaas van Schelven]

Deal w/ None as a return value of get_loader(); Django 1.4 line numbers

[Aymeric Augustin]

IMO you shouldn't do translations at compile time; you should use ugettext_lazy instead of ugettext.

It's a bit like running SQL at compile time: you can get away with it if you're lucky, but it's a bad idea.

[Klaas van Schelven]

Well yes, there are a number of things to be said against the example I came up with. Besides the use of uggettext as opposed to ugettext_lazy you could point at the fact that so much happens in init.py or the fact that one could 'work around' the problem by reversing the order of the INSTALLED_APPS.

However, I think we should strive for Django to be forgiving to its users' preferences and to fail in predictable and understandable ways. And I dare to say an ImportError should not be the perfectionists' answer to using ugettext in a class definition.

Remember the only reason Django is currently doing the import is to figure out where the apps are so it can tuck 'locale' at the end and look for .mo files there.

Doing frivolous imports in a framework is a Bad Idea (tm)

[Ramiro Morales]

Replying to vanschelven:

[...] Especially the fact that uggettext() & friends may trigger this makes this a hairy bug: uggettext is often used in the middle of a module or class definition.

FWIW, since 1.4 we have a ​note precisely about this in our i18n docs:

Lazy translation

Use the lazy versions of translation functions in django.utils.translation (easily recognizable by the lazy suffix in their names) to translate strings lazily -- when the value is accessed rather than when they're called.

These functions store a lazy reference to the string -- not the actual translation. The translation itself will be done when the string is used in a string context, such as in template rendering.

This is essential when calls to these functions are located in code paths that are executed at module load time.

This is something that can easily happen when defining models, forms and model forms, because Django implements these such that their fields are actually class-level attributes. For that reason, make sure to use lazy translations in the following cases:

• Model fields and relationships verbose_name and help_text option values
• Model verbose names values
• Model methods short_description attribute values

[Preston Holmes]

While I agree the right solution is to use the lazy translation, I believe this problem may be solved by the app-loading branch if it lands (hopefully for 1.5) as it will have imported all apps even earlier in Django's startup process.

​https://github.com/ptone/django/blob/app-loading/django/utils/translation/trans_real.py#L150

[Anssi Kääriäinen]

I tested the supplied test code in this ticket (comment:20). The problem is gone when using the app-loading branch.

However, there might be another problem: I added a debug print to trans_real.py line 150:

        print app_cache.loaded_apps

Result: [].

I do run a bare script. That is, I run the script with:

DJANGO_SETTINGS_MODULE=settings python test_trans.py

And the file contents are:

#from django.apps import app_cache
#app_cache._populate()
from defining_app import TranslatedClass

If I remove the comments from the file (ensure the app_cache is populated), I get more expected result from the debug print.

It seems there is a deeper problem in the app-loading branch, that is there is one entry point which isn't ensured to do app-loading early enough. It seems the app-loading branch could break a bunch of current applications using bare scripts.

[Anssi Kääriäinen]

Looking at the provided patch in this ticket - the pkgutil documentation says that the get_loader() will import the module if it isn't already imported. So, I fail to see what is the difference when using pkgutil compared to import_module.

To me it seems that as long as calling ugettext leads to importing other apps, the importing of other apps will lead to circular imports. Is there a way to get the module's path without doing imports?

I don't know Python's import mechanism well, so it is very much possible that I just don't understand what is happening.

[Ramiro Morales]

Replying to akaariai:

Looking at the provided patch in this ticket - the pkgutil documentation says that the get_loader() will import the module if it isn't already imported. So, I fail to see what is the difference when using pkgutil compared to import_module.

To me it seems that as long as calling ugettext leads to importing other apps, the importing of other apps will lead to circular imports. Is there a way to get the module's path without doing imports?

See ​https://github.com/django/django/pull/2049 for an attempt to achieve that using imp.find_module().

[Claude Paroz]

App loading code has now changed, at least trans_real.py does not import modules itself. See also [65cd74be8e99d06c7861edc5050e34d6444e4d56].