django-sql-explorer
django-sql-explorer copied to clipboard
chrisclark
Problem importing celery to enable tasks
Setting EXPLORER_TASKS_ENABLED = True causes the following exception:
ImportError: cannot import name 'task' from 'celery' (D:\Users\my-user\projects\my-project\venv-windows\lib\site-packages\celery\__init__.py)
Django==3.2.9
django-sql-explorer==2.4.2
celery==5.2.1
@avi-perl hi, celery 5 isn't supported. And I don't use the app with celery so updating the celery and boto parts of the app is something thats long overdue.
I would certainly appreciate a PR from anyone in a position to contribute in this area.
I've been thinking about this and I wonder if this app should just provide a set of management commands that runs any queued reports and emails them out or that push snapshots to various backends (AWS, SFTP, etc). Then people could use whatever task runner/backend storage they like? Celery is such a big dependency and (speaking for myself only) is overkill for a lot of medium traffic sites. Thoughts?
@lawson89 thats not a bad idea.
I think the biggest issue I had when I started to look at upgrading the celery support was actually the boto api usage.
If we can get that S3 functionality updated, celery 5 is fairly easy.
I can get the boto calls updated no problem - I do a lot of aws development. But just thinking it'd be nice if things were more pluggable - so when someone wants to snapshot to GCP or Azure or an sftp server it should be easy to add that. Maybe two phases? Phase 1: Get boto/celery updated so people can use that functionality Phase 2: Think about a more modular way of supporting task management & snapshot storage that doesn't lock into a particular task runner/cloud storage?
@lawson89 yeah that sounds great to me.
The actual functionality in the celery tasks can be moved into utility functions which could then be called by the task, or a management command etc. That might be the easy way to achieve it.
@marksweb ok that's a good approach - I'll start looking at the functionality. I've got 2 other PRs ahead of this but once those are in I can branch and start this refactor
@marksweb what version of celery do we want to support? 5+ Also for boto is boto3 1.24+ ok? Actually probably can support much earlier for boto3 - let me play around and see
@marksweb - the current implementation sets the object to public read (now the object has a randomized name)but I think better security would be presigned url with some reasonable expiration. Let me know what you think.
@lawson89 yeah celery 5 I think, though if we're just adding task decorators, 4 may have had shared tasks as well.
In terms of boto, I'd just go with something current. It's a project that moves constantly & I've never had issue updating it.
And yes, I agree that generating signed urls is the best way to approach S3 access. Probably also with a setting to let the project using the app define the ACL.
Ok the boto3 stuff wasn't bad - the celery stuff is proving to be a pain. Are you ok if we just do celery 5+?
I think the failing unit tests on 3.7 is a known celery issue https://stackoverflow.com/questions/73933432/django-celery-cannot-import-name-celery-from-celery-after-rebuilding-dockerf
Would appreciate a second set of eyes on this one as it's a decent amount of changes
Ok fixed the 3.7 issue by pinning a dependency I feel like this is ready for review. Unit tests are passing and I manually tested against celery4/boto3. I'll test against celery5 when I get some time