turn the models package into a facade

[mmerickel]

I'd like thoughts on this but I think it's a handy pattern for breaking apart the model definitions but bringing them together when actually used by views, etc.

[mmerickel]

ping @bertjwregeer @ergo

[mmerickel]

ping @bertjwregeer @ergo

[ergo]

I think this introduces a bit too much magic for my personal taste.
For someone unfamiliar with venusian I'm not sure what is going on. I like the fact that it removes the need to manually import things into init.py but still "Explicit is better than implicit." :-)

[ergo]

I think this introduces a bit too much magic for my personal taste.
For someone unfamiliar with venusian I'm not sure what is going on. I like the fact that it removes the need to manually import things into init.py but still "Explicit is better than implicit." :-)

[mmerickel]

Removes the chance for bugs and tediousness of remembering to add the model to __init__.py every time you create one. The code is also commented to explain what it is doing which a) works and b) teaches people a new pattern if they're interested.

[mmerickel]

Removes the chance for bugs and tediousness of remembering to add the model to __init__.py every time you create one. The code is also commented to explain what it is doing which a) works and b) teaches people a new pattern if they're interested.

[mmerickel]

There are 2 problems being solved, and not all of them are problems everyone has.

1. All models MUST BE IMPORTED in order to have them registered with the SQLAlchemy metadata.
2. It's often nice to split up the definition of the schema but bring it back together into a flat namespace when using it.

1 is the biggest source of issues when splitting up the single-module into a package of submodules. The solution is to do a scan() (or manually import all modules) prior to configure_mappers().

2 is nice and I find it logical but many people may not.

I'm open to dropping point 2 and just focusing on the scan if it helps.

[mmerickel]

There are 2 problems being solved, and not all of them are problems everyone has.

1. All models MUST BE IMPORTED in order to have them registered with the SQLAlchemy metadata.
2. It's often nice to split up the definition of the schema but bring it back together into a flat namespace when using it.

1 is the biggest source of issues when splitting up the single-module into a package of submodules. The solution is to do a scan() (or manually import all modules) prior to configure_mappers().

2 is nice and I find it logical but many people may not.

I'm open to dropping point 2 and just focusing on the scan if it helps.

[mcdonc]

My $.02... I don't mind the facade pattern, but I think the code that does this probably belongs in a library instead of in the scaffold itself. Maybe something that you can call on the configurator, e.g. configurator.make_sqlalchemy_facade('myproject.models')

[mcdonc]

My $.02... I don't mind the facade pattern, but I think the code that does this probably belongs in a library instead of in the scaffold itself. Maybe something that you can call on the configurator, e.g. configurator.make_sqlalchemy_facade('myproject.models')

[pauleveritt]

I was going to say something similar. Partially because I want to see such a library. :) But also, this is a little framework-y, and generating it in the scaffold means they are going to have to live with it. But just a minor, wafer-thin quibble.

—Paul

On Feb 1, 2016, at 3:32 PM, Chris McDonough notifications@github.com wrote:

My $.02... I don't mind the facade pattern, but I think the code that does this probably belongs in a library instead of in the scaffold itself. Maybe something that you can call on the configurator, e.g. configurator.make_sqlalchemy_facade('myproject.models')

—
Reply to this email directly or view it on GitHub #2323 (comment).

[pauleveritt]

I was going to say something similar. Partially because I want to see such a library. :) But also, this is a little framework-y, and generating it in the scaffold means they are going to have to live with it. But just a minor, wafer-thin quibble.

—Paul

On Feb 1, 2016, at 3:32 PM, Chris McDonough notifications@github.com wrote:

My $.02... I don't mind the facade pattern, but I think the code that does this probably belongs in a library instead of in the scaffold itself. Maybe something that you can call on the configurator, e.g. configurator.make_sqlalchemy_facade('myproject.models')

—
Reply to this email directly or view it on GitHub #2323 (comment).

[mmerickel]

Sure like I said I'm open to dropping the facade. Can you guys please comment on point 1 regarding making sure the modules were imported into the runtime.

def _attach_models():
    from sqlalchemy.orm import configure_mappers
    import venusian

    # ensure all models have been imported and registered with the
    # metadata prior to configuring the mapper
    scanner = venusian.Scanner()
    scanner.scan()

    # run configure mappers to ensure we avoid any race conditions
    configure_mappers()

_attach_models()
del _attach_models

This chunk of code or something like it is fairly important when splitting the models into a package.

[mmerickel]

Sure like I said I'm open to dropping the facade. Can you guys please comment on point 1 regarding making sure the modules were imported into the runtime.

def _attach_models():
    from sqlalchemy.orm import configure_mappers
    import venusian

    # ensure all models have been imported and registered with the
    # metadata prior to configuring the mapper
    scanner = venusian.Scanner()
    scanner.scan()

    # run configure mappers to ensure we avoid any race conditions
    configure_mappers()

_attach_models()
del _attach_models

This chunk of code or something like it is fairly important when splitting the models into a package.

[ergo]

def. a good idea to make sure they get imported properly.

[ergo]

def. a good idea to make sure they get imported properly.

[bertjwregeer]

I'm a big fan of this pattern. Even if it doesn't land in the scaffold I am stealing it ;-)

[bertjwregeer]

I'm a big fan of this pattern. Even if it doesn't land in the scaffold I am stealing it ;-)

[mcdonc]

If I had a sufficient stake in it, I'd probably also try to put the code in #2323 (comment) into a library as well, FWIW.

[mcdonc]

If I had a sufficient stake in it, I'd probably also try to put the code in #2323 (comment) into a library as well, FWIW.

[pauleveritt]

If this is the choice, I volunteer to write the docs. I’ve written a modest amount in this neighborhood already.

—Paul

On Feb 1, 2016, at 4:44 PM, Chris McDonough notifications@github.com wrote:

If I had a sufficient stake in it, I'd probably also try to put the code in #2323 (comment) #2323 (comment) into a library as well, FWIW.

—
Reply to this email directly or view it on GitHub #2323 (comment).

[pauleveritt]

If this is the choice, I volunteer to write the docs. I’ve written a modest amount in this neighborhood already.

—Paul

On Feb 1, 2016, at 4:44 PM, Chris McDonough notifications@github.com wrote:

If I had a sufficient stake in it, I'd probably also try to put the code in #2323 (comment) #2323 (comment) into a library as well, FWIW.

—
Reply to this email directly or view it on GitHub #2323 (comment).

[mmerickel]

Of course I've been attempting to avoid working on formalizing this pattern as @wichert already created pyramid_sqlalchemy and does a lot of this stuff using global vars versus my preferred patterns. I could submit a PR there to fix things up the way I'd do it but it'd break all of his users unsympathetically so I've avoided dealing with the topic at all. It's a lot simpler in the scope of the simplicity of the scaffold.

[mmerickel]

Of course I've been attempting to avoid working on formalizing this pattern as @wichert already created pyramid_sqlalchemy and does a lot of this stuff using global vars versus my preferred patterns. I could submit a PR there to fix things up the way I'd do it but it'd break all of his users unsympathetically so I've avoided dealing with the topic at all. It's a lot simpler in the scope of the simplicity of the scaffold.

[pauleveritt]

That’s the same thought process I think of each time I run into it. But I was thinking about something like pyramid_sqlcontext, focused on more than just SQLAlchemy wiring, to include some of the other things we wanted advocate.

On Feb 1, 2016, at 5:16 PM, Michael Merickel notifications@github.com wrote:

Of course I've been attempting to avoid working on formalizing this pattern as @wichert https://github.com/wichert already stole pyramid_sqlalchemy and does a lot of this stuff using global vars versus my preferred patterns. I could submit a PR there to fix things up the way I'd do it but it'd break all of his users unsympathetically so I've avoided dealing with the topic at all. It's a lot simpler in the scope of the simplicity of the scaffold.

—
Reply to this email directly or view it on GitHub #2323 (comment).

[pauleveritt]

That’s the same thought process I think of each time I run into it. But I was thinking about something like pyramid_sqlcontext, focused on more than just SQLAlchemy wiring, to include some of the other things we wanted advocate.

On Feb 1, 2016, at 5:16 PM, Michael Merickel notifications@github.com wrote:

Of course I've been attempting to avoid working on formalizing this pattern as @wichert https://github.com/wichert already stole pyramid_sqlalchemy and does a lot of this stuff using global vars versus my preferred patterns. I could submit a PR there to fix things up the way I'd do it but it'd break all of his users unsympathetically so I've avoided dealing with the topic at all. It's a lot simpler in the scope of the simplicity of the scaffold.

—
Reply to this email directly or view it on GitHub #2323 (comment).

[bertjwregeer]

pyramid_bestpractices :P

[bertjwregeer]

pyramid_bestpractices :P

[mcdonc]

Meh, pyramid_sqla, and a shot of tequila ;)

[mcdonc]

Meh, pyramid_sqla, and a shot of tequila ;)

[wichert]

I've seed multiple times that I would happily take a patch for pyramid_sqlalchemy which allows you to use it with a request attribute for the SQL session. I just want to also keep the current API alive as well, since I personally feel that is easier to use and works much closer to standard SQLAlchemy usage patterns and documentation. There is no reason not to have request.sql_session and pyramid_sqlalchemy.Session work.

[wichert]

I've seed multiple times that I would happily take a patch for pyramid_sqlalchemy which allows you to use it with a request attribute for the SQL session. I just want to also keep the current API alive as well, since I personally feel that is easier to use and works much closer to standard SQLAlchemy usage patterns and documentation. There is no reason not to have request.sql_session and pyramid_sqlalchemy.Session work.

[mmerickel]

@wichert This comment is not directed at you - a lot of what is below is just me sorting things in my head and trying to establish some goals.

The issue is the global metadata which is inappropriate for a third party dependency as the metadata is not abstract enough to encompass more than a single database / configuration. The better approach (imo) is to not offer any metadata at all but rather apis to attach models to an arbitrary one. The idea is to put the configuration options in the hand of the application developer, not the addon developer.

What problem are we really trying to solve? IMO it is to establish a pattern for defining models in different packages and a method for pulling them together into one or more metadata, and one or more sessions (where sessions and metadata are separately configured - pyramid_sqlalchemy makes both singular and global).

Imagine a third party who decides to depend on sqla_helper.

# no metadata/base at this time
class MyModel(object):
    id = Column(...)

Now I come in as an application developer and wish to include these models in my app's database, or a separate database (it's up to me, the application developer).

MyBase = declarative_base()
sqla_helper.attach_models_to_base(MyBase, 'addon.models')

This was recently made possible by instrument_declarative in SQLAlchemy so we just need to do a better job of embracing it. See, for example, https://bitbucket.org/miohtama/websauna/src/9820a629206fdcfb9ae4dc7d77c83c074c831b1f/websauna/system/model/utils.py?at=master&fileviewer=file-view-default#utils.py-27

Several things about this approach:

• The application developer gets final say on which models go to which databases. The addon is just coded against the fact that the models exist (probably in the same database).
• Unfortunately, it still makes the model <-> metadata coupling at a global (class definition) level but at least it is done by the application developer and they have a chance to decide. The only way around that is to make a new copy of the classes per-configuration, but I don't think anyone is excited about the prospect of bound_model = sqla_context.get_model('MyModel'); db.query(bound_model).all() where you configure an sqla_context instead of importing the models directly (but maybe it's something to consider!).
• As far as configuring the session, you should always be passing the session into the addon apis, so that shouldn't be a concern at all unless you're steadfast on using global sessions. If you are then the addon becomes coupled to one particular session and greatly reduces flexibility for the application developer.
• Something like attach_models_to_base solves the problems from this particular issue where we need to import the models and ensure they get attached to the correct metadata prior to configuring the mappers or calling create_all(). Without a separate config step that actually imports the models and attaches them there will always be extra bugs related to missing tables.
• I think the "standard SQLAlchemy usage patterns and documentation" that we are all so attached so (including me!) is simply not working for us in an ecosystem where we want to define models in third-party packages. The application developer must be able to decide which metadata and session that third-party code uses rather than being bound to a single global config with no escape-hatch.

[mmerickel]

@wichert This comment is not directed at you - a lot of what is below is just me sorting things in my head and trying to establish some goals.

The issue is the global metadata which is inappropriate for a third party dependency as the metadata is not abstract enough to encompass more than a single database / configuration. The better approach (imo) is to not offer any metadata at all but rather apis to attach models to an arbitrary one. The idea is to put the configuration options in the hand of the application developer, not the addon developer.

What problem are we really trying to solve? IMO it is to establish a pattern for defining models in different packages and a method for pulling them together into one or more metadata, and one or more sessions (where sessions and metadata are separately configured - pyramid_sqlalchemy makes both singular and global).

Imagine a third party who decides to depend on sqla_helper.

# no metadata/base at this time
class MyModel(object):
    id = Column(...)

Now I come in as an application developer and wish to include these models in my app's database, or a separate database (it's up to me, the application developer).

MyBase = declarative_base()
sqla_helper.attach_models_to_base(MyBase, 'addon.models')

This was recently made possible by instrument_declarative in SQLAlchemy so we just need to do a better job of embracing it. See, for example, https://bitbucket.org/miohtama/websauna/src/9820a629206fdcfb9ae4dc7d77c83c074c831b1f/websauna/system/model/utils.py?at=master&fileviewer=file-view-default#utils.py-27

Several things about this approach:

• The application developer gets final say on which models go to which databases. The addon is just coded against the fact that the models exist (probably in the same database).
• Unfortunately, it still makes the model <-> metadata coupling at a global (class definition) level but at least it is done by the application developer and they have a chance to decide. The only way around that is to make a new copy of the classes per-configuration, but I don't think anyone is excited about the prospect of bound_model = sqla_context.get_model('MyModel'); db.query(bound_model).all() where you configure an sqla_context instead of importing the models directly (but maybe it's something to consider!).
• As far as configuring the session, you should always be passing the session into the addon apis, so that shouldn't be a concern at all unless you're steadfast on using global sessions. If you are then the addon becomes coupled to one particular session and greatly reduces flexibility for the application developer.
• Something like attach_models_to_base solves the problems from this particular issue where we need to import the models and ensure they get attached to the correct metadata prior to configuring the mappers or calling create_all(). Without a separate config step that actually imports the models and attaches them there will always be extra bugs related to missing tables.
• I think the "standard SQLAlchemy usage patterns and documentation" that we are all so attached so (including me!) is simply not working for us in an ecosystem where we want to define models in third-party packages. The application developer must be able to decide which metadata and session that third-party code uses rather than being bound to a single global config with no escape-hatch.

[mmerickel]

Closed in favor of #2334.

[mmerickel]

Closed in favor of #2334.