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

Implement WebSockets #17

Merged
merged 7 commits into from
Oct 22, 2019
Merged

Implement WebSockets #17

merged 7 commits into from
Oct 22, 2019

Conversation

EJH2
Copy link
Collaborator

@EJH2 EJH2 commented Sep 29, 2019

This is a rudimentary attempt at implementing websockets as requested in #16. As a caveat to this, users will have to install the channels library for the websockets portion to function. A guide on what to add to your project can be found here. I didn't know how you wanted this to be documented, so I've left it alone. I have tested this on a Windows 10 machine with Python 3.6, Django 2.2, and Celery 4.3 running with Gevent. Any recommendations are welcome and heavily encouraged.

Copy link
Owner

@czue czue left a comment

Choose a reason for hiding this comment

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

Generally this looks great - thanks for the PR!

I'm a bit wary about introducing the additional dependencies for everyone that uses the lib. Do you have any thoughts/suggestions on how best to handle that? Part of me was wondering whether this should actually be a separate project, although that's probably overkill.

Also do you have any suggestions on how I can test/play with this? In full transparency I've never used channels before.

celery_progress/backend.py Show resolved Hide resolved
celery_progress/backend.py Outdated Show resolved Hide resolved
celery_progress/backend.py Outdated Show resolved Hide resolved
@EJH2
Copy link
Collaborator Author

EJH2 commented Oct 3, 2019

In complete honesty, I discovered channels in my pursuit of this project, so my knowledge of this library is quite shallow. A large chunk of this code was adapted from the tutorial for Channels, which had a strikingly similar use case to the one here. If you are interested, I can create an example project using this feature to help demonstrate it.

@czue
Copy link
Owner

czue commented Oct 4, 2019

Thanks for the super quick response. I do want to prioritize getting this supported in some way but I'm a bit swamped at the moment so I'm going to be a bit slow. 🙈

I have no idea how much work this is because I've never done it, but I think the ideal would be if this was implemented as a bundle, kind of like celery does: https://docs.celeryproject.org/en/latest/getting-started/introduction.html#bundles

If you think you can take a crack at that that'd be amazing! Else I'll put it on my backlog and revisit it as soon as I'm able.

@EJH2
Copy link
Collaborator Author

EJH2 commented Oct 13, 2019

Sorry for the late response, I had a bunch of things going on and this got lost on my to-do list! I've added the extras_require parameter to setup.py, so people can install the websockets functionality by doing pip install celery-progress[websockets,redis] (or [websockets,rabbitmq], depending on their backend of choice). I'm putting the final touches on an example project which should give you an idea of what a project using it would look like.

Edit: A bare-bones example can be found here.

@czue
Copy link
Owner

czue commented Oct 14, 2019

this is awesome! thanks so much. confirming it's ready for review/testing?

@EJH2
Copy link
Collaborator Author

EJH2 commented Oct 14, 2019

I've identified an issue where failed tasks seem to have an issue propagating to the frontend, and I'm investigating it before I give the all clear.

@EJH2
Copy link
Collaborator Author

EJH2 commented Oct 15, 2019

Both this pull request and the example project have been updated, and I believe they are ready for testing! One thing I want your opinion on though, if people have not installed the websockets part of the package, should WebSocketProgressRecorder function like it's parent class, or raise a NotImplementedError when used?

@EJH2
Copy link
Collaborator Author

EJH2 commented Oct 15, 2019

One thing I noticed is that (at least in my testing) I couldn't get (WebSocket)ProgressRecorder.stop_task to function properly. Any calls to it in a task left it completely unphased. I don't know how you would like to approach that, so I've left it untouched.

@czue
Copy link
Owner

czue commented Oct 15, 2019

Awesome, thank so much! I'll test and report back in the next couple days

Copy link
Owner

@czue czue left a comment

Choose a reason for hiding this comment

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

This is great, thanks. I haven't actually been able to test it out apart from your project, but I've confirmed that it doesn't break anything else so I'm going to merge it in, add some short docs, and then bump the version.

Thanks for the help!

@czue czue merged commit 1aab001 into czue:master Oct 22, 2019
@czue
Copy link
Owner

czue commented Oct 22, 2019

One thing I want your opinion on though, if people have not installed the websockets part of the package, should WebSocketProgressRecorder function like it's parent class, or raise a NotImplementedError when used?

I'm a bit torn on this. I added logging in #19 but I can see the case for failing harder. Since you're deliberately using the WebSocketProgressRecorder class it seems like you wouldn't necessarily want to fall back to the default behavior...

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.

2 participants