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

Pubsub implementation for easily publishing and subscribing from your resolvers #11

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

hballard
Copy link
Contributor

@syrusakbary let me know your thoughts on this pull request when you get a chance. I modified the README and all the examples (except the Django one), to show how this would work.

@kavink
Copy link

kavink commented Mar 6, 2018

@hballard Wondering if python2.7 compatibility is needed?

@hballard
Copy link
Contributor Author

hballard commented Mar 7, 2018

@kavink It should work fine now under python 2.7.

@kavink
Copy link

kavink commented Mar 8, 2018

@syrusakbary Any comments and feedback on if you would accept this in mainline ? cc: @hballard

Would be great if we get it in after whatever changes are needed.

@kavink
Copy link

kavink commented Mar 9, 2018

@hballard when I run Flask example and from graphql when I try to subscribe. I get following error

"Subscriptions are not allowed. You will need to either use the subscribe function or pass allow_subscriptions=True"

@kavink
Copy link

kavink commented Mar 9, 2018

@hballard instead of running under gunicorn, running from command line, it runs, But I dont see the results.

The subscription needs:
A subscription must return an observable

But the type it gets is <class 'graphql.execution.base.ExecutionResult'>

So I am unable to subscribe , also Error is not showing up to end user, in Graphiql I see [object Object] mostly because its returning a json instead of execution result with error.

@kavink
Copy link

kavink commented Mar 9, 2018

I think I found the issue, I had to remove all other files except keep base, constants, gevent, pubsub/gevent_observable

@hballard
Copy link
Contributor Author

hballard commented Mar 9, 2018

@kavink I need to see an example of your code to see where you are having an issue. The gevent example I modified runs fine on my machine, from the command line.

@kavink
Copy link

kavink commented Mar 9, 2018

@hballard Thanks, I got it working now from graphiql, Now im trying to make it run under foreman and gunicorn

@kavink
Copy link

kavink commented Mar 9, 2018

@hballard Awesome !!! thanks a lot for your work .. I Got it working https://bitbucket.org/noppo/gevent-websocket just had to provide worker-class. I am all set now . Thanks!

gunicorn -k "geventwebsocket.gunicorn.workers.GeventWebSocketWorker" wsgi:websocket_app

@kavink
Copy link

kavink commented Mar 9, 2018

Only annoyance right now is being required to have template.py with web socket url to make Graphiql work. And I cannot use GraphQLView.as_view

@kavink
Copy link

kavink commented Mar 10, 2018

@hballard @syrusakbary Now that I got Subscriptions working for simple example, wondering how can I do the following in RxPy, which will keep querying database using SQLAlchemy till a condition is met, I see there Observable.interval(1000) and Obervable.from_().

or maybe is it possible to plumb graphql-sqlalchemy with subscriptions so someone can be subscribed to a query ?

How can I create an Observable which will keep querying DB till condition is met and not query after condition is met, and just display last response ?

or maybe this a worth another issue for discussion in community on how to do it ?

@hballard
Copy link
Contributor Author

@kavink You can query the database for a condition, but you don't really need to. That is what you can use these classes for. Your mutations will usually be tied to updating a database. Just publish each mutation to a channel (inside your mutation resolver), after you write a mutation to your database and before it returns, and then any subscriptions subscribed to that channel (in their resolver) will automatically be updated. See this article for some good background on this concept. Probably move this to another issue (and not this pull request) if you need further discussion.

@kavink
Copy link

kavink commented Mar 13, 2018

@hballard The pubsub does not work if the definition of is a global space. I am trying to import it from a celery worker and publish it. if I define it in one file pub/sub works.Any thoughts on how can I manage the global state of subscriptions? Or maybe I shouldn't use celery and migrate to RxPy

@kavink
Copy link

kavink commented Mar 16, 2018

@hballard figured it out.. I should be using GeventRxRedisPubsub and NOT GeventRxPubsub so I can publish from celery worker and subscribe from flask/graphql in schema/subscriptions

@benwis
Copy link

benwis commented Jun 14, 2018

@hballard @syrusakbary Any chance we can know whether this is getting merged or not? I'm trying to setup subscriptions and this seems like it changes things around a fair amount

@bendemaree
Copy link
Contributor

bendemaree commented Jun 14, 2018

@hballard nice work on this; thank you! Was able to get the aio version working without much issue at all and am also hoping this can be merged soon.

The only thing I encountered was packaging-related; I think you'll want to replace the include kwarg on the find_packages line in setup.py to "everything but tests," find_packages(exclude=["*.tests", "*.tests.*", "tests.*", "tests"]) (as described in the setuptools docs), because otherwise setuptools doesn't find your new graphql_ws.pubsub package.

@hballard
Copy link
Contributor Author

hballard commented Jun 15, 2018

Thanks @bendemaree! @benwis I don't know that this "changes things around" much with the base library...you can still use the base library as is, without the classes contained in this PR. However, the PR adds simple classes that utilize a pubsub / observer pattern that you would probably need to implement yourself to get true working "subscriptions". This is a similar mechanism to what the ApolloGraphql JS subscriptions library does. @syrusakbary let me know if you'd like me to make any modifications to this PR to get it approved.

@benwis
Copy link

benwis commented Jun 15, 2018

@hballard Any chance you can tell me whether this or your graphql-python-subscriptions is the better package to use? Your documentation looks quite a bit nicer but it doesn't look like it's been updated in a while. Thanks!

@hballard
Copy link
Contributor Author

I'd use this repo, as opposed to the other one I wrote. I've pretty much deprecated it in favor of this repo once @syrusakbary released it, since it somewhat more official and a part of the graphql-python repo.

bendemaree added a commit to sportstech/graphql-ws that referenced this pull request Jul 16, 2018
@nayak16
Copy link

nayak16 commented Aug 9, 2018

@kavink How did you solve the Subscriptions are not allowed problem?

Copy link

@bdhl bdhl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check my comments

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

Successfully merging this pull request may close these issues.

6 participants