Entry point for DefaultImpl subclasses? (or, just use plugins?)

[dwreeves]

Hello,

I could not find any discussion on this elsewhere.

I'm wondering if it would make sense for Alembic to support entry-points for Defaultmpl subclasses.

---

Right now, it looks like the pattern some third-party providers use (e.g. sqlalchemy-redshift) is to load the impl as a side-effect of importing the SqlAlchemy dialect module entry-point.

There are two advantages of an entry-point. The first would be not needing to import Alembic or register anything during the normal runtime of the application. Not usually a big deal to have Alembic in the sys modules, but could be nice-to-have for custom dialects.

The second is a little more subtle, but the entry-point invocation could control the order of registration of dialects, so that the env.py's dialect is guaranteed to resolve last.

The second is actually relevant to a PR I have open for snowflake-sqlalchemy: right now there is no guarantee of whether the library provided SnowflakeImpl will resolve before or after a user's custom implementation; it's totally dependent on the env.py's imports. If entry-points are respected however, then Alembic can always resolve entry-points before env.py runs, guaranteeing user-generated DefaultImpls are resolved last. snowflakedb/snowflake-sqlalchemy#758

---

So e.g. I have a SQLAlchemy driver for MyCoolSqlDatabase, I could structure like this:

pyproject.toml
src/
  mycoolsqldatabase_sqlalchemy/
    __init__.py
    dialect.py
    alembic.py

# pyproject.toml
[project.entry-points."sqlalchemy.dialects"]
mycoolsqldatabase = "mycoolsqldatabase_sqlalchemy:dialect"

[project.entry-points."alembic.implementations"]
mycoolsqldatabase = "mycoolsqldatabase_sqlalchemy:alembic"  # alternatively mycoolsqldatabase_sqlalchemy.alembic:MyCoolSqlDatabaseImpl

---

An argument against this--

I am aware that Alembic uses entry-points for plugins.

Perhaps the pattern you might suggest is to create a plugin with an empty setup() that registers the Alembic impl as a side-effect? E.g.

# plugin.py
from alembic.ddl.impl import DefaultImpl

class MyCoolSqlDatabaseImpl(DefaultImpl):
    __dialect__ = "my_cool_sql_database"

def setup(plugin):
    pass

If so, it may make sense to show that as an example in the documentation. If that is guaranteed to resolve before env.py, then that could also work as a path for the aforementioned PR.

I think the upside to using the existing plugin ecosystem is that it reduces the API surface on Alembic's end, but the downside is this is very cheeky and not intuitive, especially without a documented example.

---

Lastly, thanks for the awesome ecosystem, zzzeeek.

[zzzeek]

yeah so i think the "plugins" system is decidedly not for dialect registration, it's for things that extend Alembic itself, typically though not necessarily in a dialect agnostic way.

So for the main question I'm sure I've thought about entrypoints for dialects, and likely felt it was a bit overkill at whatever time I thought about it and also observed that dialects like sqlalchemy-redshift (or maybe....the old DB2 dialect....i dunno, some dialect) were just putting their DialectImpl in their normal dialect and by the fact that it existed, it gets registered in the DialectImpl as an available alembic dialect.

There are two advantages of an entry-point. The first would be not needing to import Alembic or register anything during the normal runtime of the application. Not usually a big deal to have Alembic in the sys modules, but could be nice-to-have for custom dialects.

OK. meaning the alembic dialect can be totally separate from the main part of the dialect. well first off "need to import alembic", im sure all the dialects that use this pattern check if alembic is installed at all, but then if it is, sure, you end up importing alembic and I doubt we've set up alembic to be shallow-importable. so, sure I dont see another normal way around that besides entry points.

The second is a little more subtle, but the entry-point invocation could control the order of registration of dialects, so that the env.py's dialect is guaranteed to resolve last.

this one is not clicking with me. but im not thinking hard right now...how does entry point invocation control the order, why is that even important, "so the env.py's dialect"...env.py doesnt have a dialect? "is guaranteed to resolve last" ... well I dont understand "env.py's dialect" so I dont understand that either

overall, we just added entrypoints for the plugin system, which was this huge refactor I did so that we could allow this "CHECK constraint" plugin thing to be added, which we then included, and it failed immediately once we released and we basically had to almost completely revert it, it's now useless cruft in the codebase that nobody uses. But we have a plugin system as a result. So yes entrypoints are there now so my original "let's not make that big a deal" argument is pretty over, so, sure.

[dwreeves]

this one is not clicking with me. but im not thinking hard right now...how does entry point invocation control the order, why is that even important, "so the env.py's dialect"...env.py doesnt have a dialect? "is guaranteed to resolve last" ... well I dont understand "env.py's dialect" so I dont understand that either

Yeah sorry I wasn't clear about this.

snowflake-sqlalchemy was put in maintenance mode for a while but now it's active again. I've been a small-time contributor to snowflake-sqlalchemy over the past couple months. In practice I mostly end up bugfixing stuff relating to indexes because most of snowflake-sqlalchemy's maintenance predates indexes in Snowflake and I guess I'm only one of a handful of people insane enough to be using Snowflake for OLTP so I'm the one who ends up finding and fixing these bugs. I've fixed all the index related bugs I could find in the dialect and so now I've moved onto the Alembic part.

Anyway, snowflake-sqlalchemy currently tells users to add this to their env.py's:

from alembic.ddl.impl import DefaultImpl

class SnowflakeImpl(DefaultImpl):
    __dialect__ = "snowflake"

instead of having its own SnowflakeImpl.

I'm proposing to the maintainers of snowflake-sqlalchemy to add a SnowflakeImpl because Snowflake has a lot of nonstandard behaviors relating to indexes that causes problems for Alembic. (Not directly relevant to this feature request, but if you are curious, the non-stanadrd behaviors are enumerated here: snowflakedb/snowflake-sqlalchemy#758)

I'm also proposing that, for snowflake-sqlalchemy's upcoming 2.0.0 release, it's a good time to make this eagerly loaded because eagerly loading it will cause some breakage, and it's inadvisable to do this on a minor release.

What breakage? Well, if snowflake-sqlalchemy adds an eagerly-loaded SnowflakeImpl in the 2.0 release, then for existing code using the above env.py pattern, sometimes it would register the user's SnowflakeImpl, sometimes it would register the library's. Why? It depends on what imports they have at the top of their env.py. If the user never imports from snowflake_sqlalchemy, then the library's SnowflakeImpl would get registered after. (Correct me if I'm wrong on that, but I believe that's the case?)

If it were guaranteed that the library's SnowflakeImpl were always run first (e.g. via entry-point that executes before env.py), then the user's env:SnowflakeImpl would be registered last.

Without guaranteed order, this could break code, e.g.

# alembic/env.py
from alembic.ddl.impl import DefaultImpl

class SnowflakeImpl(DefaultImpl):
    __dialect__ = "snowflake"

    def some_custom_behavior_thats_important_to_me(self, *args, **kwargs):
        ...

^ If this isn't guaranteed to register last, then RIP to my some_custom_behavior_thats_important_to_me() method.

However, if the library's code always runs first, then the worst that happens is the user doesn't get the nice little fixes I'm adding, but they do keep their method overrides.

Guaranteed registration order is the important bit, so hopefully I explained that right. Saving like 100ms of start-up time and a 50kb of RAM or whatever (guesses, didnt measure) in the application runtime by removing the eager load of Alembic is cool I guess but not worth a public API feature request. The thing I really care about is making sure I can say "this won't break existing user code to add a managed SnowflakeImpl because the user's version is guaranteed to always register last."

Oh one more thing. I've been saying "look at entrypoints before running env.py" but actually it isn't strictly necessary to traverse every entrypoint prior to running env.py. What you could do instead is just say, "if dialect is not registered, then check entrypoint for [dialect name] as last resort before raising an impl not found error." Effectively that's the same behavior (i.e. prioritize user-defined impl in env.py), but it's better because you don't end up pre-loading a lot of junk and IIRC traversing entry points is surprisingly slow-ish.

overall, we just added entrypoints for the plugin system, which was this huge refactor I did so that we could allow this "CHECK constraint" plugin thing to be added, which we then included, and it failed immediately once we released and we basically had to almost completely revert it, it's now useless cruft in the codebase that nobody uses.

Oof... Sorry about that. 😓 Hopefully you got some other benefits out of it though.

[zzzeek]

What breakage? Well, if snowflake-sqlalchemy adds an eagerly-loaded SnowflakeImpl in the 2.0 release, then for existing code using the above env.py pattern, sometimes it would register the user's SnowflakeImpl, sometimes it would register the library's. Why? It depends on what imports they have at the top of their env.py. If the user never imports from snowflake_sqlalchemy, then the library's SnowflakeImpl would get registered after. (Correct me if I'm wrong on that, but I believe that's the case?)

if alembic is using an entrypoint to pull in a DialectImpl, yes this would happen when all the machinery starts up within run_migrations() or similar, because before then, it doesnt know anything about the name "snowflake". So an env.py-wide DialectImpl would already be in the registry. What happens next depends on how Alembic implements entrypoint lookups. We could either look in the DialectImpl registry first, and skip entrypoints if name is already present, or we could look up entrypoints all the time and allow them to overwrite existing ones. Or we could look up entrypoints all the time and complain that there's a conflict. OR we could look up entrypoints all the time and then also add a new method DefaultImpl.please_tell_me_what_to_do_when_there_are_conflicts() that raises by default but your snowflake thing could do whatever it wants here (like to keep your custom dialect impl add the flag USE_THIS_ONE=True).

If it were guaranteed that the library's SnowflakeImpl were always run first (e.g. via entry-point that executes before env.py), then the user's env:SnowflakeImpl would be registered last.

well it's clear some conflict reporting mechanism would be needed here and it might be just "raise an error so they can fix their env.py". all of the handwringing here over "which one goes first?" and silently steps on the other one, explicit is better than implicit. I'd not want to rely on ordering artifacts here.

However, if the library's code always runs first, then the worst that happens is the user doesn't get the nice little fixes I'm adding, but they do keep their method overrides.

just noticed the def some_custom_behavior_thats_important_to_me(self, *args, **kwargs): - two comments, first off what on earth method would they add to their impl like that, unless you mean, they are implementing their own working DialectImpl which your one one woudl fix and remove the need for those.

Guaranteed registration order is the important bit, so hopefully I explained that right. Saving like 100ms of start-up time and a 50kb of RAM or whatever (guesses, didnt measure) in the application runtime by removing the eager load of Alembic is cool I guess but not worth a public API feature request.

i'd vote for explciit conflict resolution hooks rather than guaranteed reg order.

The thing I really care about is making sure I can say "this won't break existing user code to add a managed SnowflakeImpl because the user's version is guaranteed to always register last."

if your new version is better than theirs, they should get to make that choice and I think that's what should happen here.

Having alembic's impl be "always look up an entrypoint for every DialectImpl, even if we already have one, and then use a conflict resolution hook (OK, in this part, there's some guaranteed order in that we would make sure to call the hook from the entrypoint-one first, since the entrypoint dialect is the one that was created with the knowledge of this hook, but this is a single point at which we can easily maintain this specific ordering) if there's a conflict gives the new dialect a chance to inform the user of choices via warnings or exceptions (or perhaps we should build this part into alembic). this is important to consider so it's good this is coming up.

[CaselIT]

Sadly alembic doesn't error for duplicate dialect registrations at the moment, and it's likely kinda hard to retrofit now.

(Also looking at the code the registration is still using a metaclass, we could likely move to __init_subclass__)

Anyway, snowflake-sqlalchemy currently tells users to add this to their env.py's:

That's kinda unfortunate.

Anyway, for the issue at hand my preference would be to add a different entry point for the dialect impl, but given that the registration is by subclassing we can't really prevent the current entrypont to be used this way, so maybe we could just document it?

[dwreeves]

Bouncing around responses here to form a more coherent chronology for my own message.

That's kinda unfortunate.

I agree, it's the source of the problem and if they had just done what e.g. sqlalchemy-redshift was doing then I wouldn't be here worrying about how to migrate people to the better pattern where the library gets to own the resolution to all of Snowflake's edge cases and idiosyncrasies when dealing with Alembic.

But, I also don't think conceptually having a CustomImpl subclass in env.py is the most unreasonable pattern ever. There may be users who have e.g. MyCustomPostgresImpl(PostgresImpl) or even MyCustomPostgresImpl(DefaultImpl). It would be weird if that all of a sudden stopped working.

As you say, it's hard to retrofit error for duplicate dialect registration because many people have probably built workflows around the assumption that it is not prevented.

two comments, first off what on earth method would they add to their impl like that, unless you mean, they are implementing their own working DialectImpl which your one one woudl fix and remove the need for those.

So an example I have in my own code that I'm not implementing upstream in snowflake-sqlalchemy is an override of correct_for_autogen_foreignkeys() for schema attribution. I am doing something weird that causes round-tripping in auto-migrations and I have my own weird fix that's not in the PR I have open for snowflake-sqlalchemy because I wasn't 100% confident that it should be a library behavior.

A more salient example may be, a user fixed an error with SnowflakeImpl(DefaultImpl) that I myself have not yet uncovered, so they still need this path to override the behavior while the upstream lags behind.

if your new version is better than theirs, they should get to make that choice and I think that's what should happen here.

please_tell_me_what_to_do_when_there_are_conflicts() that raises by default but your snowflake thing could do whatever it wants here (like to keep your custom dialect impl add the flag USE_THIS_ONE=True).

The suggested migration path I was thinking of was this: https://github.com/dwreeves/snowflake-sqlalchemy/blob/81ee0abfa1fb8d1da27a4b4d6e7d593cf75beccc/MIGRATING.md?plain=1#L203-L208

# env.py
from snowflake.sqlalchemy.alembic_impl import SnowflakeImpl

class CustomSnowflakeImpl(SnowflakeImpl):
    __dialect__ = 'snowflake'

    def some_custom_thing(self):
        ...

And if they don't like my version, then just keep theirs:

# env.py
from alembic.ddl.impl import DefaultImpl

class SnowflakeImpl(DefaultImpl):
    __dialect__ = 'snowflake'

    def some_custom_thing(self):
        ...

And if their version has nothing special in it (90%+ of users, probably, unless they also ran into the same issues I did and more), they should delete it:

# env.py
pass

If duplicate registration were errored, though, then this wouldn't work.

if alembic is using an entrypoint to pull in a DialectImpl, yes this would happen when all the machinery starts up within run_migrations() or similar, because before then, it doesnt know anything about the name "snowflake". So an env.py-wide DialectImpl would already be in the registry. What happens next depends on how Alembic implements entrypoint lookups. We could either look in the DialectImpl registry first, and skip entrypoints if name is already present, or we could look up entrypoints all the time and allow them to overwrite existing ones. Or we could look up entrypoints all the time and complain that there's a conflict. OR we could look up entrypoints all the time and then also add a new method DefaultImpl.please_tell_me_what_to_do_when_there_are_conflicts() that raises by default but your snowflake thing could do whatever it wants here (like to keep your custom dialect impl add the flag USE_THIS_ONE=True).

I think all of these work well for snowflake-sqlalchemy, and they all solve the problem of creating a happy path migration from env.py's to a managed Impl.

My 2 cents:

I think raising by default with please_tell_me_what_to_do_when_there_are_conflicts() is risky because of the aforementioned issue with a hypothetical user writing MyCustomPostgresImpl(PostgresImpl) into their env.py. It just doesn't seem entirely unreasonable to me that a user may already be doing that.

The others seem low risk to me.

Warning on duplicate registration is probably much better than silence. Maybe gate the warning behind a not issubclass(AlreadyRegisteredImpl, CandidateImpl) check? Because to me the intent behind a subclassed impl seems pretty explicit.

[zzzeek]

I think raising by default with please_tell_me_what_to_do_when_there_are_conflicts() is risky because of the aforementioned issue with a hypothetical user writing MyCustomPostgresImpl(PostgresImpl) into their env.py. It just doesn't seem entirely unreasonable to me that a user may already be doing that.

well that's exactly what im proposing it should do - your env.py custom code now conflicts with this new version of snowflake, you probably want to address this and either switch or use the base class or whatever. Like, this exception, or its sounding more like, a loud warning that doesnt halt execution, is intended to actively communicate to people who have this pattern and need to upgrade it.

I'd have the conflict resolution detect a user-custom extension that's on DefaulImpl, make a loud warning, then use that user-custom extension as given for that run, until they opt to change it to one of the new patterns (remove or subclass SnowflakeImpl)

Warning on duplicate registration is probably much better than silence. Maybe gate the warning behind a not issubclass(AlreadyRegisteredImpl, CandidateImpl) check? Because to me the intent behind a subclassed impl seems pretty explicit.

We'd be looking exactly for implemenations that are using the old pattern, whatever we decide that is (which it seems like, "subclasses defaultimpl directly and not the new snowflakeimpl")

[dwreeves]

Thanks for all the thoughtful discussion about this and for taking the issue seriously, btw. Really appreciate your time.

[CaselIT]

so to recap:

• we add a new entrypont explicitly to load dialects, that's different than the one used for the autogenerate plugins
• we add a check if a dialect impl is replacing another. In that case we call a classmethod please_tell_me_what_to_do_when_there_are_conflicts that by default will
  • if the new class is a subclass of the currently registered version it continue as now
  • if the new class is not a subclass of the currently registered version we warn telling users to implement the above method

does that make sense?

[dwreeves]

Makes sense. Thanks.

I will use the plugins entrypoint and a blank setup() function to ensure the SnowflakeImpl is registered, but also to ensure the user-maintained SnowflakeImpls inside the env.pys are respected. I will do this with the understanding that it is not encouraged and make sure the snowflake-sqlalchemy maintainers are aware of all of this.

The warning will also help a ton to provide telemetry when users upgrade to 2.x.

[zzzeek]

we're adding a new entrypoint though right? why wouldnt we document that?

[CaselIT]

I mean if we want to say that registering dialects using the entrypoint system is fine and not just an accident of how alembic works that's also fine by me.

[zzzeek]

im not talking about the existing entrypoints used for plugins. im talking about a new one that's consulted when alembic first loads up a dialect that would augment the logic at

alembic/alembic/ddl/impl.py

Line 139 in b42ebe1

def get_by_dialect(cls, dialect: Dialect) -> type[DefaultImpl]:

[CaselIT]

Ok, will update the text above

[dwreeves]

Thanks for your help and for the discussion everyone. Really appreciate it.

[dwreeves]

Hello, I ran into an issue pretty much immediately. 😬

alembic/alembic/runtime/plugins.py

Line 24 in b42ebe1

class Plugin:

def _setup() -> None:
    # setup third party plugins
    for entrypoint in metadata.entry_points(group="alembic.plugins"):
        for mod in entrypoint.load():
            Plugin.setup_plugin_from_module(mod, entrypoint.name)

The for mod in entrypoint.load() expects the object returned via the entry-point is iterable.

However, modules are not iterable.

The documented behavior says to set the module, but the code says to set some sort of iterable.

My suspicion is, there was a mistake in double-nesting the for loop, and nobody has raised this issue because nobody is actually using plugins.

The corrected code should look like this:

def _setup() -> None:
    # setup third party plugins
    for entrypoint in metadata.entry_points(group="alembic.plugins"):
        mod = entrypoint.load()
        Plugin.setup_plugin_from_module(mod, entrypoint.name)

In practice, for backwards compat, it should probably look like this:

def _setup() -> None:
    # setup third party plugins
    for entrypoint in metadata.entry_points(group="alembic.plugins"):
        mod = entrypoint.load()
        if isinstance(mod, Iterable):
            for o in mod:
                Plugin.setup_plugin_from_module(o, entrypoint.name)
        else:
            Plugin.setup_plugin_from_module(mod, entrypoint.name)

Or you could even reuse SQLA's utils and go with sqlalchemy.util.langhelpers:PluginLoader.

---

I think where this leaves me is that I'll end up not using Plugins after all, because this seems a little unstable and I wouldn't want to write code to match to an un-documented behavior that will likely have to be deprecated and removed at some point.

[zzzeek]

yes this is why im proposing a new entrypoint hook

[dwreeves]

I wasn't aware this is what you meant by "failed immediately."

I was just about to submit a bug report but it seems like that may not be necessary then.

[dwreeves]

In either event, it seems like with the existing plugins, there is a mismatch between docs + actual behavior. So I'll still submit the bug report.

[dwreeves]

Hello, I wrote a draft, but there is no open issue, so via the rules of the repo I cannot open a PR.

dwreeves@99622ba

Disclosure: Code is hand-written, but unit-tests are AI generated.

Let me know if I can open a PR for this.

---

A note for the resolve_registration_conflict()--

Getting this to work with just existing.resolve_registration_conflict(new) or new.resolve_registration_conflict(existing) didn't really work. The issue is:

class SnowflakeImpl(DefaultImpl):
    __dialect__ = "snowflake"

    @classmethod
    def resolve_registration_conflict(cls, other):
        print("running SnowflakeImpl.resolve_registration_conflict")
        return other

class CustomSnowflakeImpl(DefaultImpl):
    __dialect__ = "snowflake"

    @classmethod
    def resolve_registration_conflict(cls, other):
        print("running CustomSnowflakeImpl.resolve_registration_conflict")
        return other

If we call on new.resolve_registration_conflict(existing), then the existing hook cannot emit its own warnings or enforce its own pattern.

But, if we call existing.resolve_registration_conflict(new), then the reverse is true.

It seems like the only way to make sure both Impl's get to "have a say" is to have the signature be resolve_registration_conflict(cls, new: type[DefaultImpl], existing: type[DefaultImpl]) -> type[DefaultImpl] | None.

The current behavior by default mimics the existing behavior in a sense, i.e. latest registrant wins. But now both the new and existing DefaultImpl's can decide whether a new or existing can override.

However, of note the order is still non-deterministic in the sense described in the original issue because the entry point is called during context creation, not prior to env.py, so depending on the env.py's imports this could cause "unexpected" overwrites. To prioritize something in the env.py, a library author can write something like:

    @classmethod
    def resolve_registration_conflict(cls, new, existing):
        if new is cls and not issubclass(cls, existing):
            from alembic import util
            util.warn("You have a hand-written SnowflakeImpl. Please subclass the official one.")
            return existing
        return None

---

Other things of note in the commit:

• I am using SQLAlchemy's PluginLoader to consolidate entry-point handling logic. However, the PluginLoader assumes that a dialect exists; it doesn't handle the null case. So a separate cache for missing entrypoints needs to be added (_entrypoints_checked).
  • Another option would be to write a subclass AlembicPluginLoader(PluginLoader) in alembic.util that handles the misses.
• As mentioned above, __init_subclass__ hook can be used over ImplMeta. This also resolves some MyPy stuff and moreover allows for registration logic to be closer to the class.
• Added a minimal amount of docs, and I opted to not include notes about the resolve_registration_conflict in the docs as it is very in the weeds and I'm not sure most library authors will need to worry about this behavior.
• I added a NoSuchDialectError, which is of subclass (CommandError, KeyError). Previously an error on failure to load a dialect would have been a simple KeyError. (The name "NoSuchDialectError" is named to be similar to "NoSuchModuleError" in SQLA). This also allows for a slightly more direct error message than the key error when a dialect doesn't exist.

---

Again, let me know if I can open a PR. Thanks.

[zzzeek]

yes indicate Fixes: #1874 in the commit message and it will go in

#1874