Fixes: #1874 - Added entry-point for dialects

[dwreeves]

Fixes

Fixes: #1874
More discussion here: #1872

Description

This PR adds the following:

• an alembic.dialects entrypoint (named similarly to SQLAlchemy's sqlalchemy.dialects) for DefaultImpl subclasses.
• explicit conflict resolution when multiple dialects are loaded via resolve_registration_conflict classmethod
• NoSuchDialectError when dialects are not found, replacing the existing KeyError.
• Replaces ImplMeta with __init_subclass__ hook.

Background

Right now some third-party SQLA libraries use eager-loading on dialect (e.g. sqlalchemy-redshift). This change lets those libraries eventually migrate to not having to do this, which unlocks two benefits:

• has some (very minor) application runtime memory/load improvements by avoiding import alembic in the dialect.
• makes the dialect registration side-effect happen at a more predictable time (i.e. on migration context generation) by allowing the Impl to be pushed to a lazy-loaded module, rather than being unexpectedly dependent on the user's imports in env.py.

These do not directly solve the original problem of migrating users of snowflake-sqlalchemy because the entry-point is hit when the migration context is generated; meaning users generating from their env.pys; here the "last registrant wins" and the entry-point would be the last registrant. There are many solutions to the problem snowflake-sqlalchemy has, such as skip entry-points on key hit, or check for entry-points but skip loading them when they exist or use a ContextVar to prefer the env.py registrant. The solution that was agreed upon was:

• Loudly warn a user when a new DefaultImpl is not a subclass of the existing impl for that dialect (when it is subclass, the assumption is that the user's intent is clear).
• expose a "conflict resolution" classmethod, resolve_registration_conflict(new, existing), which allows either the new or existing impl to express an opinion, so to speak, and determine which one "wins" (return cls, return new, return existing all valid choices)

Notes

A note for the resolve_registration_conflict() and its function signature--

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.
• 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.
  • Let me know if you'd prefer some documentation of this. I feel like people are using AI more and the 0.01% of users for whom this is relevant can probably figure it out either on their own or with AI, but it's probably not 100% intuitive that return cls is both a valid return and also an expression of "this one (or a subclass of it) always wins."
• 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.
• _register() is guarded by if "__dialect__" in cls.__dict__ to preserve very old behavior in Alembic. At first I wasn't sure if this was here for mypy reasons vs checking cls.__dialect__ directly, but I think this line of code actually predates mypy. The difference between this and checking cls.__dialect__ directly is that wouldn't register unless classes explicitly declared a __dialect__. In practice what this means is class CustomImpl(PostgresImpl): pass does not register because __dialect__ wasn't in the __dict__. I'm assuming this is intended behavior, and even if not it's old enough + with user-facing implications that it's unreasonable to change it. So that's why _register() starts with that first check utilizing the __dict__.

---

AI disclosure: library code was hand-written, but unit-tests are 100% AI generated.

[sqla-tester]

Thanks! Issue #1874 is now marked code review in progress and no longer open for pull requests, so this pull request holds the review for it and another one won't land on top of your work. If this pull request is abandoned, a maintainer can put open for pull requests back on #1874 to reopen it to others.

[zzzeek]

good first pass, i havent looked at the tests yet

[zzzeek]

ok this is fine....but (see comment below)

[zzzeek]

this part, if we have DefaultImpl itself be a subclass of the class that has __init_subclass__ on it then we can skip this

what I'm saying is I see we got rid of ImplMeta as a type but maybe we should still have a RegisterImpl base class at the bottom of this.

[dwreeves]

Makes sense.

I got rid of ImplMeta and moved to __init_subclass__ based on the discussion; you seemed to be partial to __init_subclass__ over the metaclass.

I'll split out registration into its own thing.

One nit you can decide on, should the registry _impls be tied to the base class or keep in globals?

If former, then migration path could involve a global __getattr__(name) with if name == "_impls": warn("Don't do this"); return RegisterImpl._impls. (Or could just nix it because anyone relying on private attrs in globals gets what they have coming for them.)

[zzzeek]

I'd make it on RegisterImpl._impls ...im not sure what the backwards-compat issues would be (why would someone be accessing that)

[dwreeves]

dunno why. Maybe I'm overly cautious. I'll not make it backwards compat.

[zzzeek]

i actually will make changes like these knowing we can always fix it later, so that we actually find out it's affecting someone

[dwreeves]

Working on the changes now. RegisterImpl runs into a funny issue, 'fixable' via a type: ignore[return-value] but sort of hints toward a slightly leaky abstraction that I'm sure you're aware of but it's worth pointing out.

Basically the issue is that DefaultImpl is kind of two things in one: it's a "base class" and it's also the implementation for the "default" dialect. These are conceptually different things but right now they're used interchangeably.

---

(1) The issue is if I do this:

class RegisterImpl:
    _impls: ClassVar[dict[str, type[RegisterImpl]]] = {}

    ...

    @classmethod
    def get_by_dialect(cls, dialect: Dialect) -> type[RegisterImpl]:
        ...
        return cls._impls[dialect.name]

Then technically it returns a RegisterImpl. This breaks a lot of the typing. Tons of mypy errors if I do this with no clear remediation path other than to type[DefaultImpl]. Which is odd on a base class and also wrong.

---

(2) If I do this, however (i.e. move get_by_dialect to DefaultImpl):

class DefaultImpl(RegisterImpl):

    ...

    @classmethod
    def get_by_dialect(cls, dialect: Dialect) -> type[DefaultImpl]:
        ...
        return cls._impls[dialect.name]

Then it's still technically returning a RegisterImpl. But at least here it can be justified to add a # type: ignore[return-value] and the Mypy issues go away. Yay.

---

(3) You could always change typing to:

class RegisterImpl:
    _impls: ClassVar[dict[str, type[DefaultImpl]]] = {}

But then this leads to the problem that the cls in _register(cls) is a RegisterImpl technically. You can either type ignore that, or like, assert issubclass(cls, DefaultImpl).

---

So in all cases there is no "perfect" solution.

I went with option 2, which is to tie get_by_dialect() to DefaultImpl and just type: ignore[return-value]. This is the least obtrusive solution by my estimation.

My 2 cents is that the "real" solution to me is to have a BaseImpl class (rather than a RegisterImpl) that is dialect-less but implements interfaces for all supported public-facing API methods. Then you pick and choose how much of existing logic is split between BaseImpl vs DefaultImpl (there is an argument that DefaultImpl is essentially just a two-liner class DefaultImpl(BaseImpl): __dialect__ = "default" and all the logic pushes to BaseImpl, but you could do something in-between for sure).

But this sort of thing seems incredibly out of scope for a minor version update that all started with me wanting to safely migrate snowflake-sqlalchemy users out of their env.pys!

I'm sticking with RegisterImpl for now but just letting you know where I am here. For a brief moment in time we were net -1 type: ignore pragmas but now we're net 0. Oh well. I do think BaseImpl approach with the simple class DefaultImpl(BaseImpl): __dialect__ = "default" 2-liner both solves the leaky abstraction problem and also gets you back to -1 type ignore fwiw.

[dwreeves]

On reflection doing the two-liner isn't that crazy of a thing to do within the scope of this PR (breaking out base and default would be). I guess I just didn't imagine I'd be at this point when I first set out on figuring out how to add alembic to snowflake-sqlalchemy.

[zzzeek]

isnt this the "we made this intentional" case? issubclass(cls, existing) ? which means we would not log.debug() that same message....

[dwreeves]

Sorry, got lazy and/or too cute with it. Or not cute enough. I can remove the log.debug() or I can make the message different.

[dwreeves]

New approach. Debug log still exists (I think that's useful) but the warning-like text is only in the actual warning:

            if chosen_cls is None:
                msg = (
                    "Overwriting existing registered implementation "
                    "%r for dialect %s with new implementation %r."
                    " %r will be used."
                    % (existing_cls, dialect, newcomer_cls, newcomer_cls)
                )
                if issubclass(newcomer_cls, existing_cls):
                    # Assumed to be intentional if newcomer subclasses
                    # existing; log.debug() for telemetry.
                    log.debug(msg)
                else:
                    util.warn(
                        f"{msg} To make this intentional,"
                        " subclass the other implementation, or remove one of"
                        " them."
                    )
                chosen_cls = newcomer_cls

[dwreeves]

Thanks for the quick feedback; all very reasonable. I'll get to this in the evening. I'll leave a few notes in the meanwhile.

[dwreeves]

New head fixes all the things you went over in the comments.

• I have a note about the RegisterImpl above that may be worth looking at, both in terms of how I implemented it and also a proposal for an alternative approach.
• Double check that you're OK with the warning + log.debug approach.

Same song and dance re: AI disclosure as the first commit: hand-written library code and AI generated unit-tests. (I did review the unit-tests myself and they seem fine.)

[zzzeek]

ok let's bring it in

[sqla-tester]

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision e617c50 of this pull request into gerrit so we can run tests and reviews and stuff

[sqla-tester]

New Gerrit review created for change e617c50: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[zzzeek]

im going to try to get some alembic things happening. i have another large review i have to finish ahead of this one, let's see if i can get both

[zzzeek]

i had claude do a deep review and it seems to have identified that for a process that runs several command.XYZ() commands in one script will have somewhat randomish behavior for the conflict resolution, and that this code will generate a lot of warnings that are un-silenceable. it also found in-the-wild examples of the _impl thing. it's a lot of stuff, I can have it post review comments here for you to look at? Or I can just take it in house. ill post the comments here shortly

[sqla-tester]

AI Robot 1 (ai-robot-1) wrote:

code review left on gerrit

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[sqla-tester]

AI Robot 1 (ai-robot-1) wrote:

Not blocking: importlib.metadata could be used directly here

alembic/runtime/plugins.py reads its entry points with importlib.metadata.entry_points(group="alembic.plugins"). Doing the same for alembic.dialects would:

• remove the dependency on sqlalchemy.util.PluginLoader, which is an internal SQLAlchemy utility
• remove the need for the separate _entrypoints_checked cache, since hits and misses could be kept in one dictionary
• let the tests patch something inside Alembic instead of sqlalchemy.util.compat.importlib_metadata_get
• avoid adding the public module-level name registry to alembic.ddl.impl, which is easy to confuse with sqlalchemy.dialects.registry

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[dwreeves]

I'll remove the dep on PluginLoader. Maybe adding it in the first place was too cute and creates too much of a burden on sqlalchemy to keep it. I don't think it adds anything either way as its purely internal.

[zzzeek]

I totally missed that this used PluginLoader (or it just seemed fine to me) and that we didn't use it for the plugin plugin thing. Since we didnt use it for the plugins.py thing, then yes we should stay consistent here.

[sqla-tester]

AI Robot 1 (ai-robot-1) wrote:

The module-level alembic.ddl.impl._impls is used by third party code

Moving the registry onto RegisterImpl removes the module attribute alembic.ddl.impl._impls. During review of the pull request the maintainer advised against keeping a backwards compatible name, on the assumption that nothing outside Alembic accesses it; the maintainer has confirmed that advice was wrong. The name is private, but it is used in practice, because assigning into it is a way to point a dialect name at an existing impl class without subclassing. A GitHub code search finds, among others:

• application env.py files doing impl._impls["duckdb"] = PostgresqlImpl, and the same for other PostgreSQL or SQLite compatible databases
• the leonardovida/duckdb-sqlalchemy repository (_alembic_impl._impls.get("duckdb"))
• the test suite of cubrid-lab/sqlalchemy-cubrid (from alembic.ddl.impl import _impls)

from alembic.ddl import impl
from alembic.ddl.postgresql import PostgresqlImpl

impl._impls["duckdb"] = PostgresqlImpl
print("ok")

On main this prints ok. On this patchset it raises AttributeError: module 'alembic.ddl.impl' has no attribute '_impls', which for those users happens while env.py is being loaded.

Keeping a module-level name bound to the same dictionary avoids this at no cost, for example _impls = RegisterImpl._impls after the class.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[dwreeves]

Two options:

1. Friendly: the suggested one of _impls = RegisterImpl._impls. Silent and keeps people on the pattern indefinitely.
2. Draconian: if you want to force people off this, you can module level def __getattr__(name): ..., warn on name match, return RegisterImpl._impls.

Really depends on where you land on the friendly to draconian perspective. I err on side of friendly but 2 is an option.

[zzzeek]

yeah i'd just keep it indefinitely for now. if we want to deprecate it later, that can be..later

[zzzeek]

i wonder if we can turn _impls into a dynamic plugin registry itself. so when someone assigns into _impls it actually replaces an entrypoint that would have been there.

[sqla-tester]

AI Robot 1 (ai-robot-1) wrote:

getattr() makes subclasses that do not declare __dialect__ register themselves

This was changed from the "__dialect__" in cls.__dict__ check during review of the pull request, at the maintainer's suggestion. The maintainer has confirmed that the suggestion was wrong and that the original check should come back. The open question at the time was why the original looked only at the exact class, and the reason is this: DefaultImpl itself has __dialect__ = "default", so every subclass inherits a non-empty value and getattr(cls, "__dialect__", None) is never None. The __dict__ check was what kept helper and intermediate base classes out of the registry.

from sqlalchemy.dialects import postgresql
from sqlalchemy.engine import default

from alembic.ddl.impl import DefaultImpl
from alembic.ddl.postgresql import PostgresqlImpl


class SharedBase(DefaultImpl):
    """helper base for several impls; declares no __dialect__"""

    transactional_ddl = False


class PGFamilyBase(PostgresqlImpl):
    """helper base for PostgreSQL compatible databases; no __dialect__"""


class CockroachLikeImpl(PGFamilyBase):
    __dialect__ = "cockroachlike"


print(DefaultImpl.get_by_dialect(default.DefaultDialect()).__name__)
print(DefaultImpl.get_by_dialect(postgresql.dialect()).__name__)

On main this prints DefaultImpl and PostgresqlImpl. On this patchset it prints SharedBase and PGFamilyBase: a base class that never asked to be registered has replaced the "default" impl, and another has replaced the PostgreSQL impl for the whole process. Both replacements are silent, because the new class is a subclass of the existing one.

The "default" impl is the one alembic.testing.fixtures.op_fixture() uses unless told otherwise, so the test suites of third party dialects are affected too.

Restoring the "__dialect__" in cls.__dict__ check avoids this. The pull request description still describes that check, so the two would match again.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[dwreeves]

Sure I'll add that back.

[zzzeek]

yeah this is just, I need to spend a lot of hours focusing on a single issue to really see everything happening. that is very difficult to do these days. im not sure if this is because "everyone is submitting bugs and PRs because they have robots so there's more work to do" or "understanding problems is less fun because the robot can do it better" or what. will take us all some years to figure these things out

[zzzeek]

that was why i did a deep ai review on this one, anyway. changes that involve bootstrapping behavior always have lots of surprises that I've never been good at seeing

[sqla-tester]

AI Robot 1 (ai-robot-1) wrote:

Conflict handling at class creation changes behavior for existing users

The maintainer's direction for this change is that conflict resolution belongs only to third party dialects that ship an impl through the new alembic.dialects entry point. Until a dialect does that, no conflict resolution should happen at all, and declaring an impl class behaves as it does on main. A dialect that ships an entry point impl decides how conflicts with it are resolved. A conflict that nobody has resolved produces a warning, and a user can resolve it in favor of their own impl, after which the warning goes away.

On this patchset the conflict logic runs every time a class with a __dialect__ is created, whether or not an entry point is involved, so it already affects existing users. env.py is executed again for every command, so the class statement in it produces a new class object each time. From the second command on, the new copy is not a subclass of the copy registered by the previous command, and this warning is emitted.

import warnings

warnings.simplefilter("error")

env_py = """
from alembic.ddl.postgresql import PostgresqlImpl

class MyPostgresqlImpl(PostgresqlImpl):
    __dialect__ = "postgresql"
"""

# what ScriptDirectory.run_env() does once per command
for command in (1, 2):
    exec(compile(env_py, "env.py", "exec"), {"__name__": "env_py"})
    print("command", command, "ok")

On main this prints command 1 ok and command 2 ok. On this patchset the second iteration raises:

UserWarning: Overwriting existing registered implementation <class 'env_py.MyPostgresqlImpl'> for dialect postgresql with new implementation <class 'env_py.MyPostgresqlImpl'>. <class 'env_py.MyPostgresqlImpl'> will be used. To make this intentional, subclass the other implementation, or remove one of them.

Without warnings.simplefilter("error") it is a warning rather than an exception, but projects that run their tests with warnings as errors will see env.py fail to load. The advice in the message cannot be followed, since "the other implementation" is the same class from the previous command.

This applies to any process that runs more than one command, such as a test suite or code calling alembic.command functions, and to any env.py that declares an impl, including the class SnowflakeImpl(DefaultImpl) pattern that snowflake-sqlalchemy currently asks its users to add.

Under the direction above, _register() only needs to do what ImplMeta.__init__ did on main: store the class under its __dialect__, replacing whatever was there, with no warning and no hook call. Conflict resolution would then happen where the entry point is loaded, in get_by_dialect().

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[dwreeves]

This doesn't matter for CLI commands, it matters for calls within Python subproc command.upgrade(). And it's true. I assumed some sort of importlib thing was happening that would prevent this but I do see now that importlib.util.module_from_spec() is used and not importlib.import_module(). Good to know.

[zzzeek]

yes env.py is not treated as a module, it's treated as a python script

[zzzeek]

i told claude to propose a test suite for this issue as well, it's somewhere down below

[sqla-tester]

AI Robot 1 (ai-robot-1) wrote:

The hook should run at lookup time, on the entry point impl first and then on the user's impl

The maintainer's direction for this change is that conflict resolution belongs only to third party dialects that ship an impl through the new alembic.dialects entry point. Until a dialect does that, no conflict resolution should happen at all, and declaring an impl class behaves as it does on main. A dialect that ships an entry point impl decides how conflicts with it are resolved. A conflict that nobody has resolved produces a warning, and a user can resolve it in favor of their own impl, after which the warning goes away.

On this patchset the hook is called at class creation, on the new class and then on the existing one. Because the hook is inherited, a user's subclass of a third party impl runs the third party hook with cls set to the subclass, and a hook written in the obvious way gives the wrong answer:

from sqlalchemy.engine import default

from alembic.ddl.impl import DefaultImpl


class NewDbDialect(default.DefaultDialect):
    name = "newdb"


class LibraryImpl(DefaultImpl):
    __dialect__ = "newdb"

    @classmethod
    def resolve_registration_conflict(cls, new, existing):
        # "always keep the other implementation", written the same way as
        # the hook in test_entry_point_hook_keeps_env_py_impl
        return existing if new is cls else new


class UserImpl(LibraryImpl):
    __dialect__ = "newdb"


print(DefaultImpl.get_by_dialect(NewDbDialect()).__name__)

On this patchset this prints LibraryImpl. The user subclassed the library impl, which is the recommended way to override it, and the subclass was silently discarded. (On main it prints UserImpl, since the hook does not exist there.)

Calling the hook from get_by_dialect(), only when an entry point impl and a different, unrelated impl are both present for the dialect name, removes that problem and gives each side one clear role:

• a subclass of the entry point impl never reaches the hook, because subclassing it is the explicit override; that is the first way a user resolves a conflict in their own favor
• otherwise the hook is called on the class the entry point returned, so cls is always the third party dialect's own class and its author decides
• if that returns None, the hook is called on the user's declared impl; a user who wants to keep an unrelated impl of their own defines the hook there and returns cls, which is the second way to resolve it, and no warning is emitted
• if both return None the conflict is unresolved and a warning is emitted

With only two fixed roles the "new" and "existing" arguments are no longer needed, and the signature could be resolve_registration_conflict(cls, other), returning the class to use or None.

Which impl is used while a conflict is unresolved still needs to be stated. Keeping the impl the user declared would match what the maintainer wrote in the discussion ("make a loud warning, then use that user-custom extension as given for that run"), and is the maintainer's call.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[dwreeves]

(Let me get back to this one.)

[sqla-tester]

AI Robot 1 (ai-robot-1) wrote:

A failing entry point is reported only on the first lookup

The dialect name is added to _entrypoints_checked before registry.load() runs, so if loading the entry point raises, later lookups skip the entry point entirely.

from types import SimpleNamespace
from unittest import mock

from sqlalchemy.dialects import postgresql
from sqlalchemy.engine import default

from alembic.ddl.impl import DefaultImpl


class NewDbDialect(default.DefaultDialect):
    name = "newdb"


def load():
    raise ImportError("No module named 'somedialect.alembic_impl'")


entry_points = [
    SimpleNamespace(name="newdb", load=load),
    SimpleNamespace(name="postgresql", load=load),
]

with mock.patch(
    "sqlalchemy.util.compat.importlib_metadata_get",
    side_effect=lambda group: (
        entry_points if group == "alembic.dialects" else []
    ),
):
    for dialect in (NewDbDialect(), postgresql.dialect()):
        for attempt in (1, 2):
            try:
                result = DefaultImpl.get_by_dialect(dialect).__name__
            except Exception as err:
                result = f"{type(err).__name__}: {err}"
            print(dialect.name, "attempt", attempt, "->", result)

Output on this patchset:

newdb attempt 1 -> ImportError: No module named 'somedialect.alembic_impl'
newdb attempt 2 -> NoSuchDialectError: Implementation for dialect 'newdb' not found
postgresql attempt 1 -> ImportError: No module named 'somedialect.alembic_impl'
postgresql attempt 2 -> PostgresqlImpl

For a new dialect the second attempt hides the real error behind NoSuchDialectError. For a built-in dialect name the second attempt silently uses the built-in impl, so the broken third party impl is ignored for the rest of the process. In a test suite that means one failing test followed by tests that pass against the wrong impl.

Adding the name to _entrypoints_checked only after the load has finished would report the error every time.

Related: the value returned by registry.load() is discarded, and the lookup relies on the import having registered a class under the same name as the entry point. If the entry point is named snow but the class declares __dialect__ = "snowflake", the result is NoSuchDialectError: Implementation for dialect 'snow' not found with no hint that an entry point was found and loaded. Using the returned class would make that mistake visible, and is also what resolving conflicts at lookup time needs.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[dwreeves]

A failing entry point is reported only on the first lookup

I did this deliberately. I can change it and I think the argument of resurfacing the same error again is sound and I think I prefer that. That said, the scenarios where it matters are quite contrived; e.g. you'd have to try:except an entrypoint in one case (so that the program doesn't terminate) and then hit it again in a second case.

[zzzeek]

right but it mentions a test suite that might be doing things multiple times. I can see a lot of ways this kind of thing can go wrong and it also produces very difficult to debug situations where the same input changes what it does on successive runs. basically the method should do the same thing every time its called, so if it raises, it raises every time.

[sqla-tester]

AI Robot 1 (ai-robot-1) wrote:

Entry points should not be consulted for dialects that Alembic implements itself

On this patchset an installed package can declare an alembic.dialects entry point named after a built-in dialect, and its impl replaces the built-in one for every user of that dialect in the environment. If it subclasses the built-in impl, nothing is reported:

from types import SimpleNamespace
from unittest import mock

from sqlalchemy.dialects import postgresql

from alembic.ddl.impl import DefaultImpl


def load():
    # stands in for the module an installed package's entry point refers to
    from alembic.ddl.postgresql import PostgresqlImpl

    class SomePackageImpl(PostgresqlImpl):
        __dialect__ = "postgresql"

    return SomePackageImpl


entry_points = [SimpleNamespace(name="postgresql", load=load)]

with mock.patch(
    "sqlalchemy.util.compat.importlib_metadata_get",
    side_effect=lambda group: (
        entry_points if group == "alembic.dialects" else []
    ),
):
    print(DefaultImpl.get_by_dialect(postgresql.dialect()).__name__)

On this patchset, with warnings turned into errors, this prints SomePackageImpl. Installing a package is enough; nothing has to import it. test_entry_point_for_builtin_subclass_replaces_quietly asserts this behavior.

SQLAlchemy does not allow the equivalent. Its sqlalchemy.dialects registry resolves names registered in the running process first, then its built-in dialects, and only then entry points. Checked against SQLAlchemy main with an installed package declaring these entry points:

URL	entry point declared	resolves to
postgresql://	postgresql	the built-in dialect; the entry point is ignored
postgresql+psycopg2://	postgresql.psycopg2	the built-in dialect; the entry point is ignored
postgresql+newdriver://	postgresql.newdriver	the entry point's class

The maintainer has decided that Alembic should behave the same way: for the dialect names Alembic implements itself (default, mssql, mysql, mariadb, oracle, postgresql, sqlite), the entry point group is not consulted.

Nothing that works today is lost. A project that wants to customize a built-in impl still declares class MyImpl(PostgresqlImpl): __dialect__ = "postgresql" in its env.py, which is the counterpart of SQLAlchemy's explicit registry.register(). It also avoids a problem specific to Alembic: impls are keyed on dialect.name alone, so a third party driver such as postgresql+newdriver still has the name postgresql, and an entry point for it could not be limited to that driver.

The test named above would be inverted to assert that the built-in impl is still used.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[dwreeves]

I was actually a little worried about this one myself.

I thought about it and figured, well why not, plus I didn't consider it a security concern because anything in a Python venv is fair game, if it's there it's there. But that was the extent of my thinking.

I actually really like the note about the parallel to SQLAlchemy as I think it makes a ton of intuitive sense to mimic how SQLA does this to the extent possible. I agree with the order of operations 100%, in process -> built-in -> entry-points makes sense. In Alembic, I think # 2 comes for free with # 1 if the implementations are eagerly loaded into __init__.py.

Fixing it could be just a matter of only checking entry-points on key miss on _impls, but that depends on whether e.g. an impl in envs.py get respected over context.configure().

^ I thought I had this behavior at first? I forget what decision was made that made us check entry-points always.

[zzzeek]

yes when it found this I asked it well what does SQLAlchemy do, so that's what drove my decision that the built-in impls should not have an override path. since SQLA has been that way for 20 years and nobody has complained :)

[sqla-tester]

AI Robot 1 (ai-robot-1) wrote:

The documentation does not cover conflicts, the hook, or eager registration

This section explains how to register through an entry point but not what happens when an impl for the same dialect name already exists. That is the situation any existing third party dialect adopting the entry point will put its users in, since their env.py may already declare an impl for that name.

The changelog entry links to RegisterImpl.resolve_registration_conflict, but nothing here describes it. For authors of third party dialects it would help to state:

• that a conflict only arises for an impl loaded through the entry point, and that declaring an impl class otherwise behaves as before
• how to write resolve_registration_conflict, with an example, and what happens when it is not overridden
• that the class the entry point refers to must declare a __dialect__ equal to the entry point name
• that entry points are not consulted for the dialects Alembic implements itself
• that a dialect which ships an entry point impl should not also import that impl when the dialect module is loaded. If it does, the impl registers itself as a side effect of creating the engine, after env.py has declared the user's impl and before Alembic looks the impl up. The user's impl has then already been replaced by the time the entry point is consulted, so no conflict is seen and the hook does not run. This is how eagerly registered impls behave on main today, so it is not a regression, but the hook is only dependable for a dialect that leaves loading to the entry point

For users it would help to state the two ways to keep their own impl without a warning: subclass the dialect's impl, or define resolve_registration_conflict on their own impl.

Separately, "The RegisterImpl.__init_subclass__ hook will register the implementation" describes an internal detail; "the class is registered when it is defined" says the same thing without naming internals.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[dwreeves]

The AI has given some good feedback so far but this is the first one I'll really push back on.

My 2 cents: I have an opinion that if this behavior needs to be documented that we've failed in a sense. 99.99% of Alembic users are not writing their own Impls and 90% of people writing their own Impls shouldn't need to worry about conflict resolution. This is therefore useful to 0.001% of people, and they're probably using AI anyway. This is so incredibly in the weeds and irrelevant to almost everyone that it's embarrassing to write about it.

If I were to expand the docs I'd rather write more on just the entry-point stuff in general; I'm sure there is more to say, but maybe not so much about conflict resolution if it is designed right.

[zzzeek]

yeah this is what happens when you select "high" effort on your model....

Obviously the method itself for "resolve_registration_conflict" should have a docstring (it does). it also needs a "..versionadded:: 1.21.0" note (this will be in 1.21.0 on current schedules).

it looks like you have a short example there, so that covers "that the class the entry point refers to must declare a __dialect__ equal to the entry point name" more or less.

I think mentioning the resolve_registration_conflict() method's existence is a good idea, like "for 3rd party dialects where the userbase may have been using a recipe to handle that dialect's functionality previously, the resolve_registration_conflict hook should be considered...". I think when this hook is mentioned, or maybe in the docstring for the hook, there should be a .. note:: the conflict resolution hook will not run if the entrypoint dialect also imports itself into _impl.... . I think we need to mention that.

that is I think the resolve_registration_conflict() method's own docstring gives us a good place to note these things, and only people writing their own impl are going to find this method.

[sqla-tester]

AI Robot 1 (ai-robot-1) wrote:

Ticket number

1872 is the discussion. The issue this change fixes is 1874, so this should be :tickets: 1874 and the file should be named 1874.rst.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[sqla-tester]

AI Robot 1 (ai-robot-1) wrote:

Conflict resolution gives different results for the first command and for later commands in the same process

env.py is executed again for every command (ScriptDirectory.run_env() calls load_python_file(), which does a fresh exec_module). An impl declared in env.py is therefore a new class object on every command, and registers as the "newcomer" each time. The entry point impl is imported once per process, so it can be the newcomer at most once. Because _register() decides between "new" and "existing", the two sides swap roles after the first command. With the default rule and an env.py that declares its own impl:

command	registration event	new	existing	registered afterwards
1	env.py runs	env.py class (1st copy)	none	env.py class
1	get_by_dialect() loads the entry point	library class	env.py class	library class
2	env.py runs again	env.py class (2nd copy)	library class	env.py class
2	get_by_dialect(), name already checked	no event		env.py class
3	env.py runs again	env.py class (3rd copy)	env.py class (2nd copy)	env.py class, with a warning

A single CLI invocation makes only one decision, which is why the existing tests do not show this. It affects anything that runs more than one command per process: test suites, pytest-alembic, and programmatic use of alembic.command.

What happens on this patchset when upgrade, stamp and downgrade run in one process:

env.py declares	entry point impl	command 1	commands 2 and 3
nothing	present	library impl	library impl
class NewDbImpl(DefaultImpl)	none installed	env.py impl	env.py impl, plus an "Overwriting existing registered implementation" warning about the class replacing its own previous copy
class NewDbImpl(DefaultImpl)	present, no hook	library impl, warning	env.py impl, warning
class NewDbImpl(DefaultImpl)	present, hook keeps the env.py impl	env.py impl	env.py impl, plus the same warning. The conflict is now between two copies of the env.py class, so the library hook is not consulted
subclass of the library impl	present, no hook	env.py subclass, quiet	env.py subclass, plus the same warning, because the new copy is not a subclass of the previous copy
subclass of the library impl	present, hook keeps the env.py impl	env.py subclass	the class from the first run of env.py is reused. The subclass inherits the hook, so it runs with cls set to the new copy; new is cls is true, the new copy is not a subclass of the old copy, and the hook returns existing

Proposed tests

The tests below pin the outcome rather than the mechanism. They use the fixtures already in this file and run in offline mode, so no database is needed.

Add these imports:

import functools
from sqlalchemy.dialects import registry
from alembic import command
from alembic.testing.env import _no_sql_testing_config
from alembic.testing.env import clear_staging_env
from alembic.testing.env import env_file_fixture
from alembic.testing.env import staging_env
from alembic.testing.env import three_rev_fixture
from alembic.testing.fixtures import capture_context_buffer

Append to tests/test_dialect_registration.py:

registry.register("newdb", __name__, "NewDbDialect")


class NewDbDialect(default.DefaultDialect):
    name = "newdb"
    supports_statement_cache = True


class MultipleCommandsTest(_RegistryFixture):
    """Run several commands in one process, as test suites and programmatic
    users of :mod:`alembic.command` do.

    ``env.py`` is executed again for every command, so an implementation
    declared there is a brand new class each time, whereas a third party
    implementation is imported once per process.  The implementation that is
    selected, and the warnings emitted, should be the same for the third
    command as for the first.

    """

    env_py_impl = """
from alembic.ddl.impl import DefaultImpl

class NewDbImpl(DefaultImpl):
    __dialect__ = "newdb"

config.attributes["defined"].append(NewDbImpl)
"""

    def setUp(self):
        staging_env()
        self.cfg = _no_sql_testing_config(dialect="newdb")
        self.a, self.b, self.c = three_rev_fixture(self.cfg)

    def tearDown(self):
        clear_staging_env()

    def _library_impl(self, use_hook):
        """Return a loader standing in for the import of a third party
        module; the class is created the first time the loader is called and
        that same class is returned afterwards.

        """

        @functools.lru_cache(None)
        def load():
            if use_hook:

                class NewDbImpl(DefaultImpl):
                    __dialect__ = "newdb"

                    @classmethod
                    def resolve_registration_conflict(cls, new, existing):
                        # keep a hand written impl that predates this one
                        if new is cls and not issubclass(cls, existing):
                            return existing
                        return None

            else:

                class NewDbImpl(DefaultImpl):
                    __dialect__ = "newdb"

            return NewDbImpl

        return load

    def _write_env(self, declare_impl):
        env_file_fixture(declare_impl + """
context.configure(
    url=config.get_main_option("sqlalchemy.url"), literal_binds=True
)
config.attributes["used"].append(type(context.get_context().impl))
with context.begin_transaction():
    context.run_migrations()
""")

    def _run_three_commands(self):
        for cmd, rev in [
            (command.upgrade, "head"),
            (command.stamp, "head"),
            (command.downgrade, f"{self.c}:{self.a}"),
        ]:
            with capture_context_buffer():
                cmd(self.cfg, rev, sql=True)

    def test_env_py_impl_without_entry_point(self):
        """The only impl for the dialect is the one declared in env.py,
        which is what users of dialects that ship no impl do today.

        """
        self.cfg.attributes.update(defined=[], used=[])
        self._write_env(self.env_py_impl)

        with self._entry_points():
            with _no_warnings():
                self._run_three_commands()

        eq_(len(self.cfg.attributes["used"]), 3)
        eq_(self.cfg.attributes["used"], self.cfg.attributes["defined"])

    @testing.variation("env_impl", ["none", "legacy", "resolved", "subclass"])
    @testing.variation("library_hook", [True, False])
    def test_same_impl_for_every_command(self, env_impl, library_hook):
        library_impl = self._library_impl(bool(library_hook))
        self.cfg.attributes.update(
            library_impl=library_impl, defined=[], used=[]
        )

        if env_impl.legacy:
            # an env.py written before the third party impl existed
            declare_impl = self.env_py_impl
        elif env_impl.resolved:
            # an env.py that has decided to keep its own, unrelated impl
            declare_impl = """
from alembic.ddl.impl import DefaultImpl

class NewDbImpl(DefaultImpl):
    __dialect__ = "newdb"

    @classmethod
    def resolve_registration_conflict(cls, new, existing):
        return cls

config.attributes["defined"].append(NewDbImpl)
"""
        elif env_impl.subclass:
            # an env.py customizing the third party impl
            declare_impl = """
class CustomNewDbImpl(config.attributes["library_impl"]()):
    __dialect__ = "newdb"

config.attributes["defined"].append(CustomNewDbImpl)
"""
        else:
            declare_impl = ""

        self._write_env(declare_impl)

        unresolved_conflict = env_impl.legacy and not library_hook

        with self._entry_points(newdb=library_impl):
            if unresolved_conflict:
                with expect_warnings(OVERWRITE_WARNING):
                    self._run_three_commands()
            else:
                with _no_warnings():
                    self._run_three_commands()

        used = self.cfg.attributes["used"]
        defined = self.cfg.attributes["defined"]
        eq_(len(used), 3)

        if env_impl.none:
            eq_(used, [library_impl()] * 3)
        elif unresolved_conflict:
            # whichever side wins by default, it wins every time
            eq_(len({impl_cls.__module__ for impl_cls in used}), 1)
            if used[0] is not library_impl():
                eq_(used, defined)
        else:
            # each command uses the class that its own run of env.py
            # declared, not one left over from a previous command
            eq_(used, defined)

Run with:

pytest tests/test_dialect_registration.py -k MultipleCommands

On this patchset 4 of the 9 cases pass (the two with no impl in env.py, and the two where the env.py impl defines the hook itself) and 5 fail.

What the tests require:

• with no impl in env.py, the library impl is used for all three commands with no warnings
• with an impl in env.py and no entry point, every command uses the class declared by its own run of env.py, with no warnings, as on main
• when env.py subclasses the library impl, or the library hook keeps the env.py impl, every command uses the class declared by its own run of env.py, with no warnings
• when the env.py impl defines the hook and returns itself, the same, with no warnings; this already works on this patchset and should keep working
• when the entry point impl defines no hook and env.py declares an unrelated impl, a warning is emitted and the same side wins for all three commands

What they leave open: which side wins in that last case, and the shape of the hook. The hooks in _library_impl() and in the resolved case use the current resolve_registration_conflict(cls, new, existing) signature and would need adjusting if that changes.

How this fits the direction for the change

The maintainer's direction for this change is that conflict resolution belongs only to third party dialects that ship an impl through the new alembic.dialects entry point. Until a dialect does that, no conflict resolution should happen at all, and declaring an impl class behaves as it does on main. A dialect that ships an entry point impl decides how conflicts with it are resolved. A conflict that nobody has resolved produces a warning, and a user can resolve it in favor of their own impl, after which the warning goes away.

A prototype along those lines passes all 9 cases, and the rest of the test suite still passes with it:

• _register() does what main does: if "__dialect__" in cls.__dict__, store the class, with no warning and no hook
• get_by_dialect() loads the entry point class for the dialect name once per process, then on every call compares it with whatever is registered for that name
• if there is no entry point, or nothing else is registered, or one of the two classes is a subclass of the other, there is no conflict; the registered class or the subclass is used
• otherwise the hook is called on the entry point class; if that returns None it is called on the registered class; if both return None a warning is emitted
• entry points are not consulted for the dialects Alembic implements itself

Because the comparison is made on every lookup, between the entry point class and whatever env.py most recently declared, the result does not depend on which class happened to be created first or on how many commands have run.

With that approach these existing tests pin class-creation behavior and would change or go: the three ConflictHookTest tests test_hook_decides, test_hook_receives_new_and_existing and test_hook_returning_none_is_default, plus test_conflict_with_non_subclass_warns, test_inherited_dialect_registers, test_entry_point_conflicts_with_env_py_impl and test_entry_point_for_builtin_subclass_replaces_quietly.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7048

[zzzeek]

OK so, this is pretty complicated so let me know if you want to keep going

[dwreeves]

I can keep going when I have a second.

To be honest with you, like nearly all tech workers right now I have been slopping and vibecoding everything due to productivity pressure the last 10 months. I am jaded by it, and writing code the old fashioned way (notwithstanding unit tests) and contributing to the commons has been a joy. I'll let you know and try to be honest when I tap out but for now I'm having a lot of fun and am invigorated rather than deterred by a challenge.

[dwreeves]

AI did a good job reviewing, but it is very fragmented and doesn't provide a fuller vision of both the problem and the high-level solution.

Herein I'll refer to the nuanced stuff it uncovered as "weird behaviors." (Things I am not considering "weird behaviors": __dict__ vs getattr(), impl._impls, and the errors on entry-point thing.)

The source of the problem

First it is of note that nearly all the weird behaviors (except built-in dialect handling) are a result of DefaultImpl registration via custom subclasses within env.py specifically.

It's not even the side-effects on imports into env.py. For example, from mymodule import MyImpl just gets cached into sys.modules and all is good and well. It is specifically defining a class CustomImpl(DefaultImpl) within env.py because multiple in-process calls to an Alembic command re-runs env.py as a new module object each time. That is the source of what I am calling the weird behaviors.

There is good reason to preserve both the reloading of env.py behavior and also the registering in env.py behavior. I'm not proposing a swap to importlib.import_module() or anything similar, and I also do thing defining Impls in env.py should be supported. I just want to be clear about what I see as the source of the headaches.

Solutions

Two paths forward I think. Each choice consists of a 1st step that solves nearly everything but leaves 1 thing unresolved, then a 2nd hack-y-ish step that is required to fix the remaining issue.

1. (What I will do) Conflict resolution on entry-point only

On conflict resolution, I looked back at the original discussion and realize you suggested conflict resolution only for entry-points and I missed that, and implemented for all overwrites. My bad. I was getting some mixed messaging but I did miss that.

This leads to the 1st option:

1. "only checks conflicts on entry-point" with default behavior of "user's class always wins" (solves almost everything except the Entry points should not be consulted for dialects that Alembic implements itself issue)
2. Hard-code a list of skips or add some other mechanism exclusively to built-in Impls that skips entry-points.

This works to solve all the so-called weird behaviors unless the library overrides this to choose itself. Left untouched though, this works.

Based on previous discussion you seemed to prefer this style of approach so this approach is what I'll do.

2. (A modestly proposed alternative) Skip entry-points on _impls key hit

An alternate solution is:

1. only look at entry-points on a _impls key miss (solves almost everything except the Conflict handling at class creation changes behavior for existing users issue) (note: this includes built-in dialect fix; it comes for free)
2. Check old.__module__ == new.__module__ and old.__qualname__ == new.__qualname__ (this solves the remaining issue not solved via # 1).

The downside to this approach is it doesn't give the library a say or an opportunity to warn the user, and that was your original concern in the discussion. One could argue this is an upside though, as it gives more "control" to users (it's somewhat of an illusion, albeit a convincing one, IMO; see the "A note" section).

I think an upside is that it sort of matches how SQLA works a little more closely, in that SQLA doesn't load entry-points if a dialect is registered already in process. Although maybe you could argue SQLA should have conflict resolution such as this. SQLA also uses explicit registry.register() calls, not metaclass/init subclass hooks. So maybe apples and oranges.

Warnings on non-subclassed built-in overrides

Below is one thing neither option directly addresses, that the AI mentioned as a downside / regression from main, but I would argue can be seen as an upside. It totally depends on what you want as a behavior.

Do you want to warn on non-subclassed explicit overrides of built-ins in env.py?

See this:

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

class MyPostgresImpl(DefaultImpl):   # not a subclass of PostgresqlImpl
    __dialect__ = "postgresql"

What happens here:

• With option 2, the user gets a warning "for free" because DefaultImpl is not a subclass of MyPostgresqlImpl. But whether "for free" is good depends on whether you want the warning.
• With option 1, if you want the warning, you'd need to explicitly add it; but if you do this you still need to add the __qualname__ + __module__ check to avoid the repeat warnings issue.
• On main, right now the override happens totally silently. You could argue, maybe that's fine, it seems to be very explicit.

In both option 1 and option 2 cases, you can make minor adjustments to the code to always warn or never warn in this situation. The AI suggests preserving main branch behavior. It's unclear to me whether that was your call or the AI's.

A note

Of note, the "conflict resolution" approach gives library authors an out to implement an override that re-surfaces the weird behaviors. Technically speaking they always have the option to do this even if you do the 2nd option of skipping entry-points on key hit because you can, e.g. overwrite _register() or any other such crazy behavior. But exposing as an isolated classmethod seems to encourage it a little more. I do think this is way too in the weeds though and honestly if someone is installing some bootleg library that messes around really crazily, then maybe the user deserves it.

[zzzeek]

let's do the next revision, get all these comments resolved in one way or another, and look at what we have then. thanks!

[dwreeves]

OK great.

I'll go ahead with what I describe as the path forward in my last, longer message.

I'll try to get things in a good spot by Friday morning?

[zzzeek]

OK great.

I'll go ahead with what I describe as the path forward in my last, longer message.

I'll try to get things in a good spot by Friday morning?

sure. this is literally the only human-PR we have in the queue right now AFAICT

[dwreeves]

sure. this is literally the only human-PR we have in the queue right now AFAICT

The times are a-changing....

Before I get started on attempt 2, @zzzeek what do you think of doing this instead of the RegisterImpl: #1875 (comment) I'm not sure if it got lost in the noise but I think it's worth considering:

My 2 cents is that the "real" solution to me is to have a BaseImpl class (rather than a RegisterImpl) that is dialect-less but implements interfaces for all supported public-facing API methods. Then you pick and choose how much of existing logic is split between BaseImpl vs DefaultImpl (there is an argument that DefaultImpl is essentially just a two-liner class DefaultImpl(BaseImpl): __dialect__ = "default" and all the logic pushes to BaseImpl, but you could do something in-between for sure).

So something like this:

# All of DefaultImpl becomes BaseImpl:
-class DefaultImpl:
+class BaseImpl:

-    __dialect__ = "default"
+    __dialect__: ClassVar[str]


# DefaultImpl just becomes these two lines, that's it
+class DefaultImpl(BaseImpl):
+    __dialect__ = "default"

Here registration would be jammed into BaseImpl alongside everything else.

It solves an issue about the DefaultImpl abstraction being overloaded with two jobs, and also removes a stray type: ignore so you get "perfect" typing.

[zzzeek]

sounds great, I'd love to see it in action