Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for deep input graphs #4031

Open
devkral opened this issue Mar 18, 2024 · 4 comments
Open

Support for deep input graphs #4031

devkral opened this issue Mar 18, 2024 · 4 comments

Comments

@devkral
Copy link

devkral commented Mar 18, 2024

I currently ran in the downstream project graphql-python/graphql-core in an recursion issue (string input graphs with depths around >200 fail).

It seems to be a construction problem in the base implementation (recursive design). And the token limiting is also useless because such deep graphs will fail roughly at the same level for the js implementation.

There is a simple fix: the "stack free" (not really stack free) generator pattern, you save the current state via generator and pushes nodes and new function calls at two stacks.

here an example (python, sry not ported yet):

from functools import partial

def _foo(level=0, *, get_result):
    if level > 100000:
        yield level
        return
    print("1", level)
    yield partial(_foo, level=level + 1)
    print("2", get_result())
    yield level


def foo():
    result_stack = []

    fn_stack = [_foo(get_result=result_stack.pop)]

    while fn_stack:
        cur_el = fn_stack[-1]
        try:
            next_el = next(cur_el)
            if isinstance(next_el, partial):
                fn_stack.append(next_el(get_result=result_stack.pop))
            else:
                result_stack.append(next_el)
        except StopIteration:
            fn_stack.pop()
    return result_stack.pop()

print("final", foo())

Links:

@devkral
Copy link
Author

devkral commented Mar 20, 2024

I first misread the code. You seem to be affected the same.

One way to prevent the stack issue would be to transform many calls into generators

@yaacovCR
Copy link
Contributor

I think a max-depth guard as suggested in the linked issue sounds like a reasonable first step.

This project is open-source and so a PR to introduce “stack-free” execution should be welcome. One issue to consider is that the reference implementation should try to match the specification, which has a recursive design, but that’s not necessarily a complete block — It might depend on how transformative the PR would be. It could also depend on what performance profile the change has. If it comes with a significant improvement, for example, I imagine that it would be easier to advocate for its inclusion.

@devkral
Copy link
Author

devkral commented Apr 12, 2024

what do you think about the idea to use a rust parser for more performance and to easily synchronize the fix between both (and probably more) projects?

Rust can be compiled to webassembly so there is no compatibility issue here (and for python there are also bindings)

@devkral
Copy link
Author

devkral commented Apr 12, 2024

I lack knowledge of rust and can learn it just slowly (not much time)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants