openedx / edx-django-utils

edX utilities for Django Application development.
https://edx.readthedocs.io/projects/edx-django-utils/en/latest/
Apache License 2.0
26 stars 20 forks source link

@set_code_owner_attribute clashes with django-user-tasks #349

Open kdmccormick opened 1 year ago

kdmccormick commented 1 year ago

(I'm not sure whether this is an issue with edx-django-utils, with django-user-tasks, or both...)

Adding @set_code_owner_attribute and @shared_task to the same function causes errors when that function is an instance of UserTask.

For this reason, the import_olx and export_olx tasks in edx-platform do not use @set_code_owner_attribute: https://github.com/openedx/edx-platform/blob/db252978f3f315c896b07443f70febdc043faee4/cms/djangoapps/contentstore/tasks.py#L310-L311

I recently (accidentally) confirmed this issue... I was working on update_children_task in the content_libraries djangoapp, and had this:

@shared_task(base=LibraryUpdateChildrenTask, bind=True)
@set_code_owner_attribute
def update_children_task(self, user_id, dest_block_key, version=None):
    ....

The celery task did still work, but logs contained a background exception:

 Traceback (most recent call last):
   File "/openedx/venv/lib/python3.8/site-packages/celery/utils/dispatch/signal.py", line 276, in send
     response = receiver(signal=self, sender=sender, **named)
   File "/openedx/venv/lib/python3.8/site-packages/user_tasks/signals.py", line 215, in start_user_task
     sender.status.start()
   File "/openedx/venv/lib/python3.8/site-packages/user_tasks/tasks.py", line 84, in status
     name = self.generate_name(arguments_dict)
   File "/openedx/edx-platform/openedx/core/djangoapps/content_libraries/tasks.py", line 274, in generate_name
     key = arguments_dict['dest_block_key']
 KeyError: 'dest_block_key'

Digging in, I found that this key is missing because the django-user-tasks-defined function arguments_as_dict was not properly returning a dictionary. The underlying failure seems to be that in the call inspect.getcallargs(cls.run, ...), cls.run does not always actually equal the function defining the celery task (update_children_task), but rather the stub method Task.run. I imagine that this has to do with @set_code_owner_attribute iterfering with the introspection that @shared_task and/or django-user-tasks relies upon.

I am not exactly sure what the react or impact of this bug is. I do know for sure that developers should not use @set_code_owner_attribute on any Django user task function.

robrap commented 1 year ago

This has been a problem for certain tasks. See https://edx.readthedocs.io/projects/edx-django-utils/en/latest/monitoring/how_tos/add_code_owner_custom_attribute_to_an_ida.html#handling-celery-tasks for the alternative that avoids the decorator.

robrap commented 1 year ago

@timmc-edx: I don't know if this affects your linting.

timmc-edx commented 1 year ago

It doesn't really affect the linting in edx-platform, no -- it will accept either approach.

kdmccormick commented 1 year ago

Got it, thanks for the alternative. I see now that import_olx and export_olx both use that alternative.