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.
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 - |
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