support efficient recursion for loader options

[zzzeek]

given the test case in #8125, we can make selectinload recurse arbitrarily deep automatically like this:

diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index c4c0fb180f..68e916e7cb 100644
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -3003,6 +3003,11 @@ class SelectInLoader(PostLoader, util.MemoizedSlots):
                 ),
             )
 
+        if loadopt in new_options[0].context:
+            recursive_opt = loadopt._prepend_path_from(new_options[0])
+
+            new_options[0].context += (recursive_opt, )
+
         q = q.options(*new_options)._update_compile_options(
             {"_current_path": effective_path}
         )

that is, when it makes the new query, locate the loader option that's used for this load, then copy it with a shifted path.

the recursion is stopped for two reasons: one is once selectinload has no ids to load, it stops. so there's no extra recursion. for loops within objects, in theory, when it comes across an object again, it should be already present in the identity map and it will not try to re-load it.

if we put this under a very experimental /alpha / no guarantees kind of parameter on selectinload, we can introduce this gradually without any immediate pressure.

[0dminnimda]

Sounds cool!

[sqla-tester]

Mike Bayer has proposed a fix for this issue in the main branch:

WIP: add auto_recurse option to selectinload https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3920

[CaselIT]

I think it would make sense to bail out at certain depth. Like 10 by default making it configurable.

shall I reopen this?

[zzzeek]

not sure. you can make a chain of ten selectinload options that would do that, so if someone wants a fixed depth we already have that, though a number is easeir to enter. There is also join_depth on relationship() which does this at the class level for self-referential relationships. There's a bunch of APIs here and I think we would need to be very careful about just throwing down more keywords and such without considering the whole thing in totality. like should the option use the relatoinship-configured join_depth automatically, stuff like that.

[CaselIT]

Forgot about join_depth.
Yes I think it could use that by default. Not sure if we want to allow override of that value in the seledtin. Maybe just to allow no limit with something like auto_recurse='nolimit'?

Also reading the docs for join depth they seem to contradict each other:
Here says that when None it does not eager load, while here says that the default None is no limit

[zzzeek]

I think given that the user who wanted this ended up having a non-working use case anyway, we should keep this at one parameter and have it be something like recursion_depth, then it can be set to integer for specific depth, -1 for unlimited, None for no recursion. i dont think it's that useful that it try to read relationship(join_depth) though if it were to do that, we'd have another constant I guess. I dont really like the Union[Literal['x', 'y'], int, None] thing too much.

[zzzeek]

it doesn't say there's no limit, it says something that's hard to understand what it means, since it refers to the implementation: "When left at its default of None, eager loaders will stop chaining when they encounter a the same target mapper which is already higher up in the chain. "

so say I am Node, I'm now loading Node.children, now im on Node again, I come and see Node.children, that's "encountered the same target mapper which is already higher up in the chain". So we stop. So in that case, "join_depth" was one.

however, say I have Node, which has edges that refers to Edge, which has right_node that refers to Node. Here I load Node.edges, I can then load Edge.right_node, I come back to Node.edges, that is again "encountered the same mapper that's already higher up". So the "join_depth" is technically two here as far as how deep it's joining since it's joining Node -> Edge -> Node.

but if we look at "join_depth" as, "number of times we can load this same relationship nested", basically we can say that join_depth=None is the same as join_depth=1.

it still contradicts what's at https://docs.sqlalchemy.org/en/14/orm/self_referential.html#configuring-self-referential-eager-loading and im not sure if that's maybe how it was first implemented, though i cant find a note where it might have been changed, not sure.

[CaselIT]

Ok re-reading it it does make sense. Not sure why I read the same Instance instead of the same Mapper.

[CaselIT]

I think given that the user who wanted this ended up having a non-working use case anyway, we should keep this at one parameter and have it be something like recursion_depth, then it can be set to integer for specific depth, -1 for unlimited, None for no recursion. i dont think it's that useful that it try to read relationship(join_depth) though if it were to do that, we'd have another constant I guess. I dont really like the Union[Literal['x', 'y'], int, None] thing too much.

yes maybe using recustion_depth (technically max_recursion_depth) is the best option.
Regarding union with literal I don't mind them if there are a small number of options (1 or 2) but I agree they are not the best

[zzzeek]

OK so I think also we should add a warning to all the loaders that will emit when any kind of path is just too long. the very long paths are just not architecturally planned and someone should not be going more than 25 levels deep anywhere.

then we need to enhance the two postloaders in question here to somehow pass through the recursion depth without using the cache. what's hard there is I have to review all the ways that "current_path" is used and make sure every case is covered.

[sqla-tester]

Mike Bayer has proposed a fix for this issue in the main branch:

WIP: try to see if recursive loading can use a counter https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3931

[zzzeek]

ive reverted b3a1162 as the cache key issue makes that feature more or less a non-starter for now.

[zzzeek]

the worst case recursive test from #8142:

import sqlalchemy as sa
from sqlalchemy import orm

R = orm.registry()


@R.mapped
class N:
    __tablename__ = "n"
    id = sa.Column(sa.Integer, primary_key=True)
    parent_id = sa.Column(sa.ForeignKey("n.id"))

    children = orm.relationship("N", back_populates="parent", remote_side=parent_id)
    parent = orm.relationship("N", back_populates="children", remote_side=id)


e = sa.create_engine("sqlite:///:memory:", echo=True)

R.metadata.create_all(e)
with e.begin() as c:
    c.execute(
        N.__table__.insert(),
        [{"id": i, "parent_id": i - 1 if i > 0 else None} for i in range(200)],
    )


def stack_selectinload(depth: int):
    if depth == 1:
        return orm.selectinload(N.children)
    return stack_selectinload(depth - 1).selectinload(N.children)


def stack_joinedload(depth: int):
    if depth == 1:
        return orm.joinedload(N.children)
    return stack_joinedload(depth - 1).joinedload(N.children)


def stack_subqueryload(depth: int):
    if depth == 1:
        return orm.subqueryload(N.children)
    return stack_subqueryload(depth - 1).subqueryload(N.children)


with orm.Session(e) as s:
    # fine
    # stmt = sa.select(N).filter(N.id == 1).options(stack_joinedload(50))
    # sqilte bails but it generates quickly
    # stmt = sa.select(N).filter(N.id == 1).options(stack_joinedload(200))

    # does not cache at all. always generated. fast
    #stmt = sa.select(N).filter(N.id == 1).options(stack_selectinload(5))
    # does not cache at all. always generated. fast
    # stmt = sa.select(N).filter(N.id == 1).options(stack_selectinload(25))

    # does not cache at all. always generated. starts slowing
    # stmt = sa.select(N).filter(N.id == 1).options(stack_selectinload(50))
    # does not cache at all. always generated. slowing
    # stmt = sa.select(N).filter(N.id == 1).options(stack_selectinload(75))
    # does not cache at all. always generated. slow
    # stmt = sa.select(N).filter(N.id == 1).options(stack_selectinload(100))
    # does not cache at all. always generated. very slow

    stmt = sa.select(N).filter(N.id == 1).options(stack_selectinload(150))
    # does not cache at all. always generated. very very slow.
    # Also does ~10 fast than pause for some time (guessing gc). large memory use 2gb
    # stmt = sa.select(N).filter(N.id == 1).options(stack_selectinload(150))

    # also slow (also sqlite bails at 64 tables)
    # stmt = sa.select(N).filter(N.id == 1).options(stack_subqueryload(63))
    # very slow (also sqlite bails at 64 tables)
    # stmt = sa.select(N).filter(N.id == 1).options(stack_subqueryload(200))

    # Not tried the other loaders

    result = s.execute(stmt)
    print(result.unique().all())

    result = s.execute(stmt)
    print(result.unique().all())

[zzzeek]

so i still held off on making this work for more than immediately self-referential relationships, i think overall this option might not be that popular anyway because this has never really been asked for. however the idea is that asyncio might make it more popular than before.

[CaselIT]

Have you managed to fix also the memory usage?

[zzzeek]

if recursion_depth is used, then it certainly should. the mem issue was from generating a long line of hundreds of loader options that were then multiplied into that many Load objects. then cache keys had to be generated from each of those, the whole thing went into the cache, the 1G thing is very plausible.

the implementation now uses just one Load object with two elements inside that are fixed, paths dont grow beyond four tokens, the first statement is cached, and that's it.

if you dont do recursion_depth and actually string together dozens of loader objects, something I dont think anyone was doing other than me, then the memory use is still very wasteful but at least it turns off caching so it's not nearly as bad.

[zzzeek]

small regression caused by this issue:

from sqlalchemy import *
from sqlalchemy.ext.hybrid import hybrid_property
from sqlalchemy.orm import *
Base = declarative_base()


class A(Base):
    __tablename__ = 'a'

    id = Column(Integer, primary_key=True)
    name = Column(String)
    bs = relationship("B", lazy="selectin")


class B(Base):
    __tablename__ = 'b'
    id = Column(Integer, primary_key=True)
    a_id = Column(ForeignKey("a.id"))
    data = Column(String)

e = create_engine("sqlite://", echo=True)
Base.metadata.create_all(e)



s = Session(e)

a1 = A(bs=[B()])
s.add(a1)
s.commit()

s.expire(a1, ["name"])
s.refresh(a1, ["name", 'bs'])

Traceback (most recent call last):
  File "/home/classic/dev/sqlalchemy/test4.py", line 34, in <module>
    s.refresh(a1, ["name", 'bs'])
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/session.py", line 2696, in refresh
    loading.load_on_ident(
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/loading.py", line 478, in load_on_ident
    return load_on_pk_identity(
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/loading.py", line 613, in load_on_pk_identity
    return result.one()
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/engine/result.py", line 1673, in one
    return self._only_one_row(
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/engine/result.py", line 715, in _only_one_row
    row: Optional[_InterimRowType[Any]] = onerow(hard_close=True)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/engine/result.py", line 1531, in _fetchone_impl
    return self._real_result._fetchone_impl(hard_close=hard_close)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/engine/result.py", line 2125, in _fetchone_impl
    row = next(self.iterator, _NO_ROW)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/loading.py", line 216, in chunks
    post_load.invoke(context, path)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/loading.py", line 1410, in invoke
    loader(
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/strategies.py", line 1414, in _load_for_path
    state.get_impl(key).set_committed_value(state, dict_, value)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/attributes.py", line 1975, in set_committed_value
    collection.append_multiple_without_event(value)
  File "/home/classic/dev/sqlalchemy/lib/sqlalchemy/orm/collections.py", line 567, in append_multiple_without_event
    for item in items:
TypeError: 'LoaderCallableStatus' object is not iterable

[sqla-tester]

Mike Bayer has proposed a fix for this issue in the main branch:

include pk cols in refresh() operations unconditionally https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4311