Resolve #160 -- Add picture_processed signal
#231
Conversation
codingjoe
left a comment
codingjoe
left a comment
There was a problem hiding this comment.
Cool, that's an interesting suggestion. I wonder if we can take this a step further and have a persistent "processed" state on the model.
Maybe similar to the dimension fields…
I want to make sure that this package is pretty convenient by default and then allows you to grow and adapt behavior to your needs.
This doesn't need to be implemented into the library necessarily, but we should at least add a cookbook on how to use the signal for this use case.
pictures/tasks.py
Outdated
| file_name: str, | ||
| new: list[tuple[str, list, dict]] | None = None, | ||
| old: list[tuple[str, list, dict]] | None = None, | ||
| field: str = "", |
There was a problem hiding this comment.
All that matters for async task runners, is that the signature is JSON-serializable.
You don't need to concatenate the strings; just pass a triple:
| field: str = "", | |
| sender: tuple[str, str, str], |
You can drop the default, since this will be required. And I'd prefer to keep the naming somewhat consistent. Thus, this would be the sender (sending the task).
There was a problem hiding this comment.
Making it required means that it needs to be placed before new and old, which is more of a breaking change than making it optional. It does make upgrading more difficult as existing celery tasks without the kwarg being set could be on users queues, so they would end up being rejected.
Now that we're sending along the field as the sender the storage could also be dropped as that could be found in the task from the field, but that would cause issues on upgrading again due to it being set as a kwarg to the celery tasks.
Having sender as optional would allow a more graceful upgrade path, they could then be made required in a further major release. Same with making storage optional for now before removal in a further major release.
There was a problem hiding this comment.
Hm… but we'd always include a sender in the function call. Thus, custom processors would immediately break, right?
There was a problem hiding this comment.
In the codebase process_picture is called with args, not kwargs, so the calls would break with TypeError: process_picture() takes 4 positional arguments but 5 were given and the tasks would not be scheduled. That would be caught in any CI test that assigns an image to a picture field.
Our problem would be on a higher level and only occur when deploying a new version. The tasks from the older version of Django pictures would have already been created with 4 kwargs (storage, filename, new (optional), old (optional)) and are sat on the message broker. With an active site it is not feasible to ensure that none of the old tasks exist on the queue when upgrading. You then deploy a new version and the workers now only accept 5 kwargs (storage, filename, sender, new (optional), old (optional)). There, Celery will reject the message as sender is not set, raise an exception and put it back on the queue as a new message.
Maybe we have Celery misconfigured in some way but the worker will then pick the message up again, and repeat. We end up flooding Sentry, and the only way around it from that point is to create our own celery task that can handle both types of message on the queue.
codingjoe
changed the title
process_picture_done signalpicture_processed signal
codingjoe
changed the title
picture_processed signalpicture_processed signal
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #231 +/- ##
===========================================
- Coverage 100.00% 99.40% -0.60%
===========================================
Files 13 14 +1
Lines 495 505 +10
===========================================
+ Hits 495 502 +7
- Misses 0 3 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jmsmkn
force-pushed
the
process_picture_signal
branch
from
e54b332 to
ed11b97
Compare
Cleanup sets instance to a FakeInstance, so needed to find the model through another path.
Thanks for the feedback, I think this is ready for another look. If this is the correct approach I wonder if a second signal could be useful - |
| from django.dispatch import receiver | ||
|
|
||
| from pictures import signals, tasks | ||
| from tests.test_migrations import skip_dramatiq |
There was a problem hiding this comment.
This should probably be moved somewhere else. Otherwise, module-level fixtures will be unintentionally loaded too and introduce side effects.
There was a problem hiding this comment.
Sorry, but I do not see how this is an issue. The module, and its potential fixtures, are already loaded when you run pytest. I do not see how to move this method either without duplicating the multiple if statements for the imports.
|
|
||
| @receiver(signals.picture_processed, sender=Profile._meta.get_field("picture")) | ||
| def picture_processed_handler(*, sender, file_name, **__): | ||
| sender.model.objects.filter(**{sender.name: file_name}).update( |
There was a problem hiding this comment.
Hm… we should probably pass the model instance to the signal, just like the post- or pre-save signals.
Your filename doesn't need to be unique. It should, but it really doesn't have to.
However, this would mean fetching it from the DB in the processing task. I'd love to avoid DB IO in the processing task by default.
If you add a unique constraint and index, including a comment on why they matter, that might be the best solution. What do you think?
There was a problem hiding this comment.
An index makes sense for the lookup here, but I'm not sure if a unique constraint is strictly necessary. filter will update all rows that match. I think that makes sense as I think that the variations would have been removed for that file.
To send along the instance we would need acceess to it to serialize it. Accessing the instance in https://github.com/jmsmkn/django-pictures/blob/4077289ae6a75c8f8a810d04487af4a95b4f1a45/pictures/models.py#L179-L185 doesn't work as Django cleanup assigns a fake instance to self.instance. I don't think that there is another workaround for that through self.field.
There was a problem hiding this comment.
Pull request overview
This PR adds a new picture_processed signal that is emitted when asynchronous picture processing tasks complete. This allows users to track processing status and perform actions (like updating database flags) when images finish processing.
- Introduces the
picture_processedsignal in a newpictures/signals.pymodule - Adds a
senderparameter (tuple of app_label, model_name, field_name) to all picture processing functions to enable signal emission with proper field context - Provides comprehensive test coverage for the new signal functionality
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pictures/signals.py | Defines the new picture_processed signal |
| pictures/tasks.py | Updates all picture processing functions to accept and use the sender parameter, and emits the picture_processed signal after processing completes |
| pictures/models.py | Adds a sender property to PictureFieldFile that returns model/field metadata as a tuple for signal emission |
| tests/test_signals.py | Adds comprehensive tests for the signal functionality including verification of signal emission and object retrieval |
| tests/test_tasks.py | Updates existing test to pass the new required sender parameter |
| tests/test_migrations.py | Adds @isolate_apps decorator to migration tests and imports the utility |
| README.md | Documents the new signal feature with usage examples showing how to track picture processing status |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| assert not migration.to_picture_field.called | ||
|
|
||
| @pytest.mark.django_db | ||
| @isolate_apps |
There was a problem hiding this comment.
The @isolate_apps decorator requires an argument specifying the app label(s) to isolate. It should be used as @isolate_apps('app_label') or @isolate_apps('app1', 'app2'). Without an argument, this will not work as intended.
| @isolate_apps | |
| @isolate_apps("testapp") |
There was a problem hiding this comment.
Tried that initially, doesn't work.
| assert not luke.picture | ||
|
|
||
| @pytest.mark.django_db | ||
| @isolate_apps |
There was a problem hiding this comment.
The @isolate_apps decorator requires an argument specifying the app label(s) to isolate. It should be used as @isolate_apps('app_label') or @isolate_apps('app1', 'app2'). Without an argument, this will not work as intended.
| @isolate_apps | |
| @isolate_apps("testapp") |
There was a problem hiding this comment.
Tried that initially, doesn't work.
| @@ -0,0 +1,3 @@ | |||
| import django.dispatch | |||
|
|
|||
| picture_processed = django.dispatch.Signal() | |||
There was a problem hiding this comment.
The picture_processed signal should include a docstring documenting its parameters (sender, file_name, new, old) and their types/meanings. This helps users understand what data is available when handling the signal.
| picture_processed = django.dispatch.Signal() | |
| picture_processed = django.dispatch.Signal() | |
| picture_processed.__doc__ = """ | |
| Signal sent when a picture has been processed. | |
| Parameters: | |
| sender: The sender of the signal (usually the model class). | |
| file_name (str): The name of the processed picture file. | |
| new (bool): True if the picture is newly processed, False if updated. | |
| old (bool): True if the picture existed before processing, False otherwise. | |
| """ |
There was a problem hiding this comment.
Can be done when the discussion about the method signature is resolved.
2 participants
This PR implements a new signal,
process_picture_done, which is emitted when the task completes. It adds a new kwarg toPictureProcessorinstances,field, which is a string so that it can be serialized in Celery and contains the model name, app label and field name. This, combined with the file name, allows the user to find which instance(s) this processing task corresponds to, so that they could then take action, e.g. by setting a processing done field in the database.Any feedback welcome!
See #160