-
Notifications
You must be signed in to change notification settings - Fork 90
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
Websocket Optimization #51
Conversation
…hing to a homegrown and more performant solution here
…mewhere internally to not break the entire system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the change to return the meta
object!
Had some questions mostly around the behavior when things aren't set up. Will give you a chance to respond before approving.
'Tried to use websocket progress bar, but dependencies were not installed / configured. ' | ||
'Use pip install celery-progress[websockets] and set up channels to enable this feature. ' | ||
'See: https://channels.readthedocs.io/en/latest/ for more details.' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that anything that ever imports from this file will raise an exception here if things aren't configured, yeah?
I'm not sure that's a problem, but it feels a bit less risky if the error gets raised when you actually try to use the class, not just import it. Is there a reason you didn't do it that way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will error if the default channel layer has not been configured. I originally thought it would be best to catch the error before it happens, and only evaluate whether the channel layer exists once vs doing it every time a task is updated, but I understand if the other change would make more sense. Although, I now notice that that error will do nothing anyways, as it's not raised at all. How I managed to remove raise during testing and not put it back before committing I will never know. I will push a new commit moving it back into WebSocketProgressRecorder
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you rather it stay as a logging error, or as an actual error?
As a logger error in __init__
, the message would fire once for task, and the except AttributeError
would silently drop all messages. As a real error in __init__
, attempting to use the progress bar would just kill the task and put a traceback in the celery console.
Another interesting interaction that I've noticed is that currently, if a user has channels properly set up with the exception of having a channel layer named "default" configured, attempting to connect to the websocket via JS will throw AttributeError: 'NoneType' object has no attribute 'group_add'
. This is due to the fact that, by default, the consumer will look for the "default" channel layer, and fail if it can't be found. Should we handle this as well?
) | ||
except AttributeError: # No channel layer to send to, so ignore it | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this path reachable anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the default channel layer cannot be found, and due to the lack of properly raising the error as explained above, the AttributeError exception will still be triggered by the post-run handler.
One thing I've been thinking of is that currently, the nature of get_channel_layer()
just looks for a channel layer named "default", and if none can be found, returns None instead. Should we offer the ability to configure the name of this layer, so that other layers are not potentially impacted? I would investigate that as another PR if so, as to not clog this one with features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never actually used the websocket implementation - so it's not a hugely informed opinion - but at a high level configuring it by name sounds useful.
I also like this change but I have two concerns:
|
Omar, your concerns are certainly warranted, but the reason for this change is this:
|
That makes sense.
My proposal is to separate data collection from message generation. Data collection: In the case of HTTP the data would be collected by AsyncResult. And in WS, by Message generation: Both HTTP and WS would pass the data to a common function that generates the message to send to the client (like
Sorry I meant |
I would be unopposed to a potential implementation if it makes developing and maintaining this easier for us.
From what I can tell at this point, if |
I'll try to come up with something tomorrow. |
I've gone ahead and changed the behavior of the error down to a logger warning inside of As an aside, would it be possible to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good from my side. feel free to go ahead and merge if you're happy with it.
) | ||
except AttributeError: # No channel layer to send to, so ignore it | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never actually used the websocket implementation - so it's not a hugely informed opinion - but at a high level configuring it by name sounds useful.
Sure. How do I do this, exactly? Or are you able to? |
thanks - done! |
@OmarWKH If you're ok with this, I'll go ahead and merge it in which would allow you to continue development on your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good
This pull request includes several changes:
WebSocketProgressRecorder
without proper configuration now results in aRuntimeError
ProgressRecorder
'sset_progress
andstop_task
functions now return the task metadataProgress(task_id).get_info()
to get the same infoWebSocketProgressRecorder
now takes advantage of the aforementioned extensionAsyncResult
instance just to fill a dictionary with basic infoAny reviews would be greatly appreciated.