Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1277 (closed defect: fixed)

Opened 12 years ago

Last modified 11 years ago

Buffet adapters get configuration data too late

Reported by: cwells Owned by: chrisz
Priority: high Milestone: 1.0.4
Component: TurboGears Version: 1.0
Severity: major Keywords: buffet config
Cc:

Description

The current implementation initializes the Buffet adapters prior to completely parsing the TurboGears configuration files. This means that most of the configuration is unavailable to template engines until render time, which is highly inefficient.

In order to get around this flaw, the current implementation also has Genshi and Kid-specific configuration hard-coded in which is inflexible, violates DRY, and makes life more difficult for users of alternate template engines.

What I'd suggest instead is that there be an additional section added to the TurboGears config specifically relating to template engine (i.e. [templates]). Such a section might look something like:

[templates]
genshi.encoding = 'UTF-8'
breve.root = 'myproject/templates'

The loader would then instantiate the template engine passing in only the relevant sections, so for instance, Genshi would see {'encoding':'UTF-8'} and Breve would see {'root':'myproject/templates'} as their "config" parameter to init.

The following patch to turbogears.view.base.load_engines isn't pretty but works:

def _load_engines():
     if engines: return

     from turbogears.util import get_project_config
     this_project = os.path.basename(os.getcwd()) + '.'
     cfg_file = get_project_config()
     config_obj = turbogears.config.config_obj(cfg_file, this_project)
     engine_options = config_obj [ 'templates' ]
     for entrypoint in 
pkg_resources.iter_entry_points("python.templating.engines"):
         cfg_prefix = entrypoint.name + '.'
         this_engine_opts = dict (
             [ (k[len(cfg_prefix):],v)
               for k,v in engine_options.items()
               if k.startswith(cfg_prefix) ]
         )
         engine = entrypoint.load()
         engines[entrypoint.name] = engine(stdvars, this_engine_opts)

This patch probably isn't the correct long-term solution (a better solution would be to actually parse the config file prior to initializing the template engine layer), but at least provides temporary relief.

Change History

comment:1 Changed 12 years ago by jorge.vargas

  • Priority changed from normal to high
  • Owner changed from jorge.vargas to anonymous
  • Component changed from unassigned to TurboGears

comment:2 Changed 12 years ago by alberto

  • Milestone changed from 1.1 to __unclassified__

Batch moved into unclassified from 1.1 to properly track progress on the later

comment:3 Changed 12 years ago by chrisz

  • Keywords buffet config added
  • Owner changed from anonymous to chrisz
  • Version changed from trunk to 1.0
  • Severity changed from normal to blocker
  • Milestone changed from __unclassified__ to 1.0.4

Unfortunately, this ticket mixes two issues.

I think the main issue that buffet adapters get configuration data too late should be solved immediately (in 1.0) (workarounds have already been implemented in #458 and #1145). That's why I have reopened the ticket.

The second suggestion, that a buffet adapter shouldn't get the parameters for the other buffet adapters, but only its own parameters, does also make a lot of sense. But this can be solved later (later than 1.0) because of the involved compatibility issues.

My use case is passing parameters to TurboCheetah?.

My solution for the first issue is to simply not call load_engines() in view/base.py since this is called in startup.startTurboGears() anyway.

I have committed this in r3685 already, but it seems so simple that there may be a snag in it. Let me know if you see any problems with this.

comment:4 Changed 11 years ago by faide

  • Milestone changed from 1.0.4 to 1.1

The suggestion of having the loader send only the required information to each buffet adapter is quite valid. This should be implemented in 1.1. BUT the proposed solution should not be used as is because using os.getcwd() makes no sense when inside an egg (which is the intended way of deployement for a TG app).

Specific methods from pkgresources should be used instead.

comment:5 Changed 11 years ago by Chris Arndt

  • Severity changed from blocker to normal

Changing severity, since this is not blocking TG operation, it's just inefficient.

comment:6 Changed 11 years ago by chrisz

  • Severity changed from normal to major

On the mailing list, a problem with r3685 was posted when mixing huge sets of Kid + Genshi projects (two distinct projects where one - with Genshi as default - was a subset of another - with Kid as default). I'll examine this thoroughly and modify the patch if necessary.

(BTW, I think I meant "too early" when I wrote "too late" in the comment 11/17/2007.)

comment:7 Changed 11 years ago by chrisz

  • Milestone changed from 1.1 to 1.0.4

comment:8 Changed 11 years ago by godoy

With this patch in place I had problems when mixing huge sets of Kid + Genshi projects (yes, two distinct projects where one -- with Genshi as default -- is a subset of another -- with Kid as default).

The traceback ended with:

File "/home/godoy/desenvolvimento/python/TurboGears/1.0/turbogears/widgets/base.py",
line 232, in __call__
  return self.display(*args, **params)
File "/home/godoy/desenvolvimento/python/TurboGears/1.0/turbogears/widgets/meta.py",
line 108, in lockwidget
  output = self.__class__.display(self, *args, **kw)
File "/home/godoy/desenvolvimento/python/TurboGears/1.0/turbogears/widgets/base.py",
line 272, in display
  return view.engines.get('kid').transform(params, self.template_c)
AttributeError: 'NoneType' object has no attribute 'transform'

Just readding the line "load_engines()" by the end of the file fixed this problem all the way up to HEAD as it is today.

I suggest that the removal of this last line be reversed until we can be sure that this won't affect projects that mixes things like default template systems.

comment:9 Changed 11 years ago by godoy

TurboGears Complete Version Information

TurboGears requires:

Toolbox Gadgets

Identity Providers

tg-admin Commands

Visit Managers

Template Engines

  • genshi-markup (Genshi 0.4.4)
  • genshi-text (Genshi 0.4.4)
  • genshi (Genshi 0.4.4)
  • mako (Mako 0.1.9)
  • pylonsmyghty (Pylons 0.9.6.1)
  • json (TurboJson? 1.1.2)
  • cheetah (TurboCheetah? 1.0)
  • kid (TurboKid? 1.0.4)

Widget Packages

  • dynwidgets (dynwidgets 1.1)
  • checkselect (checkselect 1.2)
  • quadro_horario (quadro-horario 1.0)
  • submodal (submodal 1.5)
  • dominclude (DOMinclude 1.0)
  • tinymce (TurboTinyMCE 1.0.6)
  • scriptaculous (Scriptaculous 1.8.0)
  • selectshuttle (Select-Shuttle 1.3.1)

TurboGears Extensions

comment:10 Changed 11 years ago by godoy

I believe that it is because of the mixed default templating system for each project and subproject.

The architecture is:

  • BIG project using Kid (there was no Genshi when this started)
  • Several (up to now 12) small projects as "modules" using Genshi

BIG project is a master project, with specific functionality and modules while small projects are modules that can be shared among different BIG projects (there are 3 BIG projects sharing 12 small projects).

All of them have custom widgets, custom templates and BIG projects have a master.kid and a master.html with the equivalent Genshi, so that I can use XPath to include everything and keep the same layout, no matter if it is a module inside BIG project or if it is a whole small project.

Small projects are runnable per se if I add identity classes to their model, with an uglier layout but have all functionality.

Each project has a "model.py" specific to what it needs, an up to date dev.cfg and prod.cfg, etc. There's no duplicate content for the database functions or database maintenance scripts.

This is a preparation to have smaller specialized and modular applications that will be doing specific tasks and can be maintained in their own schedule without impacting the critical "BIG" application.

There is no trick to do this. On this specific BIG app I was testing I have, for one small app:

# -*- coding: utf-8 -*-

import logging

from turbogears import pkg_resources
from turbogears.view.templates import sitetemplate
from fornecedores import controllers as fornecedores

log = logging.getLogger("siteamostras.controllers.fornecedores")


class Fornecedores(fornecedores.Fornecedores):

   templates = pkg_resources.resource_filename('siteamostras', 'templates/master.html')

   def __init__(self, *args, **kword):
       super(Fornecedores, self).__init__(masters=[self.templates], log=log, *args, **kword)

Then I instantiate each module on the small app passing the master template and log instance to use or using the default one (ugly, but it works):

# -*- coding: utf-8 -*-

import logging

import turbogears
from turbogears import controllers, expose
from turbogears import identity

log = logging.getLogger("fornecedores.controllers")


from fornecedores.controllers.cad_fornecedores import Fornecedores as CadFornecedores
from fornecedores.controllers.contatos import Contatos

class Fornecedores(controllers.Controller):

   def __init__(self, *args, **kword):
       global log
       super(Fornecedores, self).__init__(*args, **kword)
       self.masters = kword.get('masters', ['master.html'])
       log = kword.get('log', log)

       self.fornecedores = CadFornecedores(masters=self.masters, log=log)
       self.contatos = Contatos(masters=self.masters, log=log)

WSGI will make it simpler, I hope, but so far I had no time to port it.

comment:11 Changed 11 years ago by chrisz

I have sent you an email with my assumption what is causing the problem, can you confirm?

comment:12 Changed 11 years ago by chrisz

Ok, so it seems the problem is caused by code in a controller on the module level that amounts to the following (which provokes the same exception):

w = widgets.JSSource('foo')
x = w()

Calling a widget instance w means actually rendering the widget, which needs the templating engine, which is not yet initialized at that point in time due to r3685 which causes the exception.

There are two ways of solving this problem:

1) Treat this as an error, because normally you only want to render a widget after the application has started, so calling w() rather indicates a programming mistake, e.g. you think that w is a widget class that you try to instantiate while in reality it is already a widget instance.

If we treat this as an error, then the error message should be improved and say something helpful like "you're trying to render a widget, but the templating engine is not yet loaded".

2) Providing "preliminary" widget engines with default options until the application is started. After the application is started, the engines are loaded again with the correct options.

Solution 2) actually amounts to adding the line load_engines() in view/base.py again, as suggested above. In this case, the templating engines will be instantiated twice, first with the default options, and then later on startup, with the correct options of the application, and the old instances will be thrown away.

If we go with 2) and add the load_engines() call to the code again, then we should also add a comment explaining why we do so.

Not sure which behavior we actually want. Maybe there are use cases for rendering widgets before the application has started?

comment:13 Changed 11 years ago by Chris Arndt

+1 on option 1)

BTW: often, when rendering a widget, you need access to the request object anyway, which would not be available at application startup time too.

comment:14 Changed 11 years ago by chrisz

Another argument for 1) is that you are still free to call load_engines() on your own in your start script or root controller module if you really need this. We could mention this possibility in the error string and/or the docstring of load_engines.

comment:15 Changed 11 years ago by godoy

I believe that this last comment -- the possibility of calling load_engines() on the root controller module or startup script -- nails the problem with a definitive answer.

With regards to the problem, I could remove the instantiation of some widgets and I have seen no collateral effect so far. Some of them still have to be instantiated but are working fine.

I believe then that the best thing is making a better error message and adding a comment on views/base.py that if this needs to be done then it can be done on the startup script or root controller module.

I am happy with this enhancement and my problem is solved, so far.

Thank you very much for your help.

comment:16 Changed 11 years ago by khorn

option 1) makes more sense to me

much better than reloading the template engines twice

comment:17 Changed 11 years ago by chrisz

  • Status changed from new to closed
  • Resolution set to fixed

I have checked in a patch for option 1) as r3968 now.

Note: See TracTickets for help on using tickets.