NEXT icon indicating copy to clipboard operation
NEXT copied to clipboard

update to be compatible with newest version of celery

Open dconathan opened this issue 8 years ago • 8 comments

A recent version of celery broke our implementation, hence celery==3.1.25. It's possibly just a refactoring of some method names, but it could be more.

dconathan avatar Dec 15 '16 22:12 dconathan

Celery has a guide on going from 3.1 to 4.0: http://docs.celeryproject.org/en/latest/whatsnew-4.0.html#upgrading-from-celery-3-1

stsievert avatar Dec 15 '16 23:12 stsievert

I figured it out. They changed how the prefetch multiplier option is set. It's an easy fix. I'll submit a PR to fix it soon.

They are also switching all the upper case constants/settings to lower case... (backwards compatible for now but are recommending switching "as soon as possible". This will involve some more substantial (but hopefully harmless) changes to constants/launch scripts/docker-compose files...

I'll test this out tomorrow...

dconathan avatar Jan 25 '17 04:01 dconathan

Please keep us in the loop on this. It could have large ramifications for some current experiments. On Jan 24, 2017 20:34, "dconathan" [email protected] wrote:

I figured it out. They changed how the prefetch multiplier option is set. It's an easy fix. I'll submit a PR to fix it soon.

They are also switching all the upper case constants/settings to lower case... (backwards compatible for now but are recommending switching "as soon as possible". This will involve some more substantial (but hopefully harmless) changes to constants/launch scripts/docker-compose files...

I'll test this out tomorrow...

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nextml/NEXT/issues/159#issuecomment-275016849, or mute the thread https://github.com/notifications/unsubscribe-auth/ABWhGA7bvfYBdyBs581tbM42Nuz022plks5rVtC7gaJpZM4LOplV .

lalitkumarj avatar Jan 25 '17 08:01 lalitkumarj

Okay. Like I said, there are two changes here:

  1. To fix compatibility with 4.0, we need to make very minor changes to next_worker_startup.sh
  2. To maintain compatibility with future versions, refactor the constants as lower-case. I haven't looked into the exact scope, but this would change at least a few files, and would require some testing to make sure celery is picking up our options and not just using its own defaults somewhere.

However, I just tested and making the 1st change DOES break 3.1.25 (presumably the 2nd does too), so either of these changes will require updating celery/rebuilding the containers.*

So... do we want to hold off on this upgrade and incorporate it into the next major NEXT release?

*Containers can be rebuilt from cache by making the change in the requirements.txt file and just running docker-compose build ... it only takes a few seconds... something to keep in mind.

dconathan avatar Jan 25 '17 14:01 dconathan

I've got a PR ready for this. I'm going to wait until some of the existing PRs get merged so things don't get crazy. Details will come but I made it so it's backwards compatible so we can roll back if necessary. I'm not updating constants yet because that breaks too many things with 3.1.25.

dconathan avatar Apr 03 '17 18:04 dconathan

@dconathan We're contemplating a release soon-ish; can you open a PR with that code?

erinzm avatar Jun 29 '17 19:06 erinzm

Backed off on this change for now. Some comments @dconathan made on Slack about why we're not doing this:

I have moved to celery 4. I have noticed getQuery has been pushing .5-1s delays for no apparent reason and I haven't gotten around to debugging it... this could explain that.

Tomorrow I'll rebuild with celery 3.1.25 and see if that fixes it... Just reverted back to celery 3.1.25 and getQuery delays are back down to sub .1s range… so almost certainly seeing a slowdown due to celery 4

I was wondering about it. Almost all the extra time was “enqueued” time according to the charts. ... I guess there’s one reason not to switch to the latest and greatest

erinzm avatar Aug 24 '17 18:08 erinzm

@dconathan can you test this again, since it's apparently fixed (c.f. celery/celery#3814)

erinzm avatar Oct 23 '17 18:10 erinzm