Override Task.apply() to support eager running of Batches
#16
Conversation
|
On the surface this change looks pretty reasonable. I'll need to compare it with the implementation of a "normal" task and see if it will break at all. Any way of adding a test for this? |
|
I'm not sure, how are the tests run at the moment? Vanilla `pytest`?
…On Wed, 11 Dec 2019, 16:11 Patrick Cloke, ***@***.***> wrote:
On the surface this change looks pretty reasonable. I'll need to compare
it with the implementation of a "normal" task and see if it will break at
all.
Any way of adding a test for this?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#16>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXCKYSMS4DNGYRDLUPIQKTQYEGMRANCNFSM4JZBDXSA>
.
|
|
Pretty much. The current ones are a little weird cause they run a separate worker in a different thread, I think. Which shouldn't be necessary for an eager task though! |
|
Yeah, I've been reading about the celery plugin of pytest. It should be
easy enough to decorate a test function with the `task_always_eager`
config. Hopefully I'll have some time to knock it out tomorrow.
…On Wed, 11 Dec 2019, 20:20 Patrick Cloke, ***@***.***> wrote:
Pretty much. The current ones are a little weird cause they run a separate
worker in a different thread, I think. Which shouldn't be necessary for an
eager task though!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#16>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXCKYVQ44LZFM3QVW2XGW3QYFDP7ANCNFSM4JZBDXSA>
.
|
|
Hi @clokep, do you know if there is likely to be any move to merge this at some point? (just reflagging in case it has been forgotten, I apologise if there is some process this is moving through outside of GitHub). |
|
@scalen Sorry for the delay! Lost track of this over the holidays. I'll try to get to this "soon". 😄 Please feel free to ping me again if you don't hear back soon! I think the code looks pretty good, but I wanted to test this in our development environment first to make sure the change of the base task doesn't break anything. |
|
Makes sense to me 🙂 Take your time, of course, just wanted to make sure it
was still on your radar!
…On Thu, 9 Jan 2020, 13:26 Patrick Cloke, ***@***.***> wrote:
@scalen <https://github.com/scalen> Sorry for the delay! Lost track of
this over the holidays. I'll try to get to this "soon". 😄 Please feel
free to ping me again if you don't hear back soon!
I think the code looks pretty good, but I wanted to test this in our
development environment first to make sure the change of the base task
doesn't break anything.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXCKYSIYE22JGNNIRVZRMTQ44QYXANCNFSM4JZBDXSA>
.
|
Prior to this change, eager running of batches caused errors whereby the batched task was passed the args and kwargs directly, rather than wrapping them as a request, when the following was called: `.apply_async(args, kwargs, *_args, **_kwargs)` This change updates the minumum viable version of celery, and overrides the new `Task.apply()` method to pass the request wrapped in a `SimpleRequest` and a `list` to the underlying function when in eager mode.
|
Something shook loose while reviewing this again and I realized I had worked on this previously! See 80609f8 for what I had come up with. It is very similar, but I think the key you unlocked was inheriting from the correct Task class! I'm testing this out, but I think it looks good overall! 👍 |
|
OK! This looks good. I have a couple of commits to add, but GitHub is complaining about me pushing to your branch, so I'll add them on top afterward. |
|
See #18 for the changes I made. |
Minor clean-ups to eager code path (PR #16)
Prior to this change, eager running of batches caused errors whereby
the batched task was passed the args and kwargs directly, rather than
wrapping them as a request, when the following was called:
.apply_async(args, kwargs, *_args, **_kwargs)This change updates the minumum viable version of celery, and overrides
the new
Task.apply()method to pass the request wrapped in aSimpleRequestand alistto the underlying function when in eager mode.