Pylon's integration of SQLAlchemy config extremely broken
Michael Bayer
with create_engine() needs a serious overhaul, small as it may be.
id like to apply the "pool_recycle" argument to create_engine().
however pylons.database provides no way to propigate arbitrary keys
from the .ini file to create_engine(); its hardcoded to just "dburi"
and "echo". Additionally, if I stick a pylons.database.create_engine
() statement inside of config/environment.py, pylons.database will
cache that particular engine based on its URI as well as the string
version of its **kwargs, therefore pylons.database.session_context
still has no way to get at those options, since it doesnt have those
**kwargs with which to construct the key. but not only that, its not
even caching it in the same dictionary that the running application
will use.
i tried using this code in environment.py:
kwargs = {}
uri, echo = database.get_engine_conf()
kwargs['echo'] = echo
database.get_engines()['%s|%s' % (uri, str(kwargs))] =
database.create_engine(uri, echo=echo, pool_recycle=7200)
which would at best be a horrible kludge to get this working, as i am
imitating a private key-making function to make it work. but even
*that* doesnt work, because get_engines() at that point returns the
_db_engines dict thats inside of database.py, but when the app
actually runs, it returns the _db_engines thats on top of "g".
setting it within lib/app_globals.py doesnt do any better. So I have
to stick it inside of lib/base.py which is not at all where
environmental options should be configured.
design changes i would make:
- definite: have just *one* dictionary which stores database engines,
not two. database._db_engines and g._db_engines, if you even need
g._db_engines at all (why is it needed in two places?) should be *the
same dictionary*. having two of the same thing is useless and
creates ambiguity.
- no reason why not: alter pylons.database.create_engine() to
provide a "config_key" argument, which specifies exactly what key a
particular database engine will be stored under. put this key as
in .ini as well under sqlalchemy.key or similar so it can be affected
from both places.
- nice to have: allow support for *any* keyword under
sqlalchemy.XXXXX to be propigated from the .ini file to create_engine
(), not just 'uri' and 'echo'. have awareness of the well known
kwargs so that types can be changed from strings to ints/booleans as
needed.
Philip Jenvey
On May 25, 2007, at 10:23 AM, Michael Bayer wrote:
>
> Forgive my attention grabbing headline but I think Pylons integration
> with create_engine() needs a serious overhaul, small as it may be.yes
Oh yes. Don't worry, I hate it too
We were storing it under g as g is specific to your application (you
might have more than one Pylons apps deployed in the same process).
The database._db_engines came about because g isn't initialized in
many cases (like websetup.py). I suppose having all engines cached in
the module dict wouldn't hurt -- but we have to keep in mind that
there may be multiple Pylons apps.
We need to make it easier for things like websetup.py to
automatically have access to g, and any StackedObjectProxy that is
around during the request.
> - no reason why not: alter pylons.database.create_engine() to
> provide a "config_key" argument, which specifies exactly what key a
> particular database engine will be stored under. put this key as
> in .ini as well under sqlalchemy.key or similar so it can be affected
> from both places.
I've been meaning to switch to using an identifier. Like,
sqlalchemy.readonly.dburi = 'sqlite://memory'
sqlalchemy.readonly.pool_size = 2
And maybe the default of no identifier would use sqlalchemy.dburi and
the like.
> - nice to have: allow support for *any* keyword under
> sqlalchemy.XXXXX to be propigated from the .ini file to create_engine
> (), not just 'uri' and 'echo'. have awareness of the well known
> kwargs so that types can be changed from strings to ints/booleans as
> needed.
>
How would one easily setup non int/bool/str options to pass to
create_engine? Like import pyodbc; module=pyodbc, or poolclass=Pool
()? If we want to maintain the default
pylons.database.session_context, you need to intercept the keyword
arguments passed along to create_engine with python code. Maybe with
a setuptools entry point?
I've been avoiding any big changes to the SQLAlchemy support stuff
until it's fully sorted out.
--
Philip Jenvey
Michael Bayer
On May 25, 2007, at 3:12 PM, Philip Jenvey wrote:
>> - nice to have: allow support for *any* keyword under
>> sqlalchemy.XXXXX to be propigated from the .ini file to create_engine
>> (), not just 'uri' and 'echo'. have awareness of the well known
>> kwargs so that types can be changed from strings to ints/booleans as
>> needed.
>>
>
> How would one easily setup non int/bool/str options to pass to
> create_engine? Like import pyodbc; module=pyodbc, or poolclass=Pool
> ()? If we want to maintain the default
> pylons.database.session_context, you need to intercept the keyword
> arguments passed along to create_engine with python code. Maybe with
> a setuptools entry point?
the non-primitive types of arguments to create_engine() i think need
to be supported by an API call. like database.setup_engine
(config_key, **kwargs) or something. that will place an appropriate
engine in the get_engines() dict (or just the arguments somewhere
that will be used in a subsequent create_engine() call).
python is great as a configurational language so i dont think
the .ini file should have to be where *everything* goes.
not sure what you mean by the setuptools entry point part, though.
sayrer
>
> - nice to have: allow support for *any* keyword under
> sqlalchemy.XXXXX to be propigated from the .ini file to create_engine
> (), not just 'uri' and 'echo'. have awareness of the well known
> kwargs so that types can be changed from strings to ints/booleans as
> needed.
Yes, I really need this too.
Ben Bangert
> Forgive my attention grabbing headline but I think Pylons integration
> with create_engine() needs a serious overhaul, small as it may be.
Agreed, and I'm working on that. :)
> which would at best be a horrible kludge to get this working, as i am
> imitating a private key-making function to make it work. but even
> *that* doesnt work, because get_engines() at that point returns the
> _db_engines dict thats inside of database.py, but when the app
> actually runs, it returns the _db_engines thats on top of "g".
> setting it within lib/app_globals.py doesnt do any better. So I have
> to stick it inside of lib/base.py which is not at all where
> environmental options should be configured.
Fixed in latest Pylons trunk with new pylons.config object (there
when the app is initialized and not running, AND there when the app
is running).
> design changes i would make:
>
> - definite: have just *one* dictionary which stores database engines,
> not two. database._db_engines and g._db_engines, if you even need
> g._db_engines at all (why is it needed in two places?) should be *the
> same dictionary*. having two of the same thing is useless and
> creates ambiguity.
Done in the latest Pylons trunk. Definitely a nice change as well.
> - no reason why not: alter pylons.database.create_engine() to
> provide a "config_key" argument, which specifies exactly what key a
> particular database engine will be stored under. put this key as
> in .ini as well under sqlalchemy.key or similar so it can be affected
> from both places.
I'll be implementing this as Phil suggested, such that adding another
name with a dot in the config will add more engine's available under
that config key. Ie:
sqlalchemy.ro_only.dburi = .....
etc.
> - nice to have: allow support for *any* keyword under
> sqlalchemy.XXXXX to be propigated from the .ini file to create_engine
> (), not just 'uri' and 'echo'. have awareness of the well known
> kwargs so that types can be changed from strings to ints/booleans as
> needed.
This will be part of the above implementation for the additional
engine config with a key for usage.
Cheers,
Ben
Mike Orr
> Fixed in latest Pylons trunk with new pylons.config object (there
> when the app is initialized and not running, AND there when the app
> is running).
"Now you see it, now you do."
>
> > design changes i would make:
> >
> > - definite: have just *one* dictionary which stores database engines,
> > not two. database._db_engines and g._db_engines, if you even need
> > g._db_engines at all (why is it needed in two places?) should be *the
> > same dictionary*. having two of the same thing is useless and
> > creates ambiguity.
>
> Done in the latest Pylons trunk. Definitely a nice change as well.
>
> > - no reason why not: alter pylons.database.create_engine() to
> > provide a "config_key" argument, which specifies exactly what key a
> > particular database engine will be stored under. put this key as
> > in .ini as well under sqlalchemy.key or similar so it can be affected
> > from both places.
>
> I'll be implementing this as Phil suggested, such that adding another
> name with a dot in the config will add more engine's available under
> that config key. Ie:
>
> sqlalchemy.ro_only.dburi = .....
> etc.
>
> > - nice to have: allow support for *any* keyword under
> > sqlalchemy.XXXXX to be propigated from the .ini file to create_engine
> > (), not just 'uri' and 'echo'. have awareness of the well known
> > kwargs so that types can be changed from strings to ints/booleans as
> > needed.
>
> This will be part of the above implementation for the additional
> engine config with a key for usage.
Michael and I are still planning on a SAContext object which will make
the model metadata-centric rather than sessioncontext-centric, while
still dispensing sessions as needed. I guess this can go on top of
your multi-engine config-parsing code. It will probably obviate the
need to store engines under 'g' at all: engines will be in a SAContext
object in the model. It's pretty straightforward, maybe I can get it
done this week.
Is it OK to go to sqlalchemy.default.dburi for the main engine and
sqlalchemy.other_db.dburi for additional engines? That would cut out
a bunch of complexity parsing options both with and without a middle
part.
--
Mike Orr <slugg...@gmail.com>
Ben Bangert
> Michael and I are still planning on a SAContext object which will make
> the model metadata-centric rather than sessioncontext-centric, while
> still dispensing sessions as needed. I guess this can go on top of
> your multi-engine config-parsing code. It will probably obviate the
> need to store engines under 'g' at all: engines will be in a SAContext
> object in the model. It's pretty straightforward, maybe I can get it
> done this week.
The 'g' thing is gone already in trunk. Having it fixed at the
metadata also means you can use autoload=True I assume? Right now one
needs to import create_engine and connect it before a table statement
including autoload can even exist (which kind of sucks).
> Is it OK to go to sqlalchemy.default.dburi for the main engine and
> sqlalchemy.other_db.dburi for additional engines? That would cut out
> a bunch of complexity parsing options both with and without a middle
> part.
I didn't see anything about who was working on a SAContext or its
progress, which was the main motivation for me to start adding more
methods into Pylons to handle loading configs well. Let me know the
scope of what SAContext will cover so I can avoid any replication of
work.
Cheers,
Ben
Mike Orr
> progress, which was the main motivation for me to start adding more
> methods into Pylons to handle loading configs well. Let me know the
> scope of what SAContext will cover so I can avoid any replication of
> work.
Michael and I have agreed on an implementation design so I just have
to implement it. He's busy with other tickets and documentation right
now. I'm planning to use Michael's API but change the constructor a
bit to handle multiple databases in the config file.
We need a generic config function that returns all config vars under a
certain prefix as a dict. Both SAContext and my Buffet replacement
need that.
def get_options(options_dict, prefix, strip_prefix=True):
# options_dict is app_conf.
prefix += "."
ret = {}
for key in options_dict.iterkeys():
if key.startswith(prefix):
newkey = key[prefix:]
ret[newkey] = options_dict[key]
return ret
I'm not quite sure how to handle the subkey (the middle part of
"sqlalchemy.myengine.dburi").
On the one hand we could parse only a certain specified subkey. On
the other hand we could set up engines for all subkeys found in one
step, which would require either a function that lists the subkeys, or
one that returns a two-level dict of subkey : option : value. This is
why going to mandatory subkeys for "sqlalchemy.default.dburi" is
appealing: it avoids having to write two sets of code for whether the
subkey is present. I'm also thinking about making a logging
initializer that would work similarly: "logging.realm.subrealm.level =
info", something like that.
Another thing that's necessary is a standard way to preconvert into
and boolean values. asbool works but there's no asint, and int() is
exception-prone. I favor preconverting values and setting defaults at
the beginning of the application, because if the conversion is buried
in every function that reads a config value, you'll get exceptions
happening at obscure times. Better to have all exceptions occur at
startup as much as possible. I was thinking of something like this:
def convert_options(app_conf, bool_options=['echo', 'echo_pool'],
int_options=['pool_recycle']):
# Modify app_conf in place.
This gets problematic for setting defaults and ignoring prefixes
though. Maybe a one-liner for each option:
convert_option(app_conf, option_name, converter, default=NoDefault,
prefix=None):
# Convert option in place or raise exception.
# Converter is a function like int or bool (which can be a signal
to use asbool instead)
# Exception if option is missing and NoDefault.
# If prefix is "sqlalchemy" and option_name is "pool_recycle",
operate on all options
# with that prefix and suffix regardless of subkey.
If you can work on optiion-parsing functions, I'll work on the rest.
I actually think these option-parsing functions belong in paste.deploy
because they're useful in all applications, but first they have to
stabilize.
> The 'g' thing is gone already in trunk. Having it fixed at the
> metadata also means you can use autoload=True I assume? Right now one
> needs to import create_engine and connect it before a table statement
> including autoload can even exist (which kind of sucks).
Or use session_context.current.bind_to to get the engine, which sucks
almost as badly. Then users wonder if the engine is thread safe (it
is) because they define their tables at the global scope but use them
in various other threads.
The idea is to pair every engine with a BoundMetaData so they act as a
unit. The SAContext acts as a container for engine-metadata pairs,
with one being the default. sac.session will be a property for the
current session, using a private SessionContext. sac.query will
(maybe?) be a shortcut for sac.session.query. This should cover
almost all user applications, including those that do weird things
like a transaction spanning multiple databases.
--
Mike Orr <slugg...@gmail.com>