resolve #305 abstract models #314

jose-padin · 2022-05-12T16:43:37Z

Added support for abstract models. #305

AllexVeldman

I'm not entirely clear on how this change is intended to be used.

When implementing a custom TaskResult model, how would you tell django_celery_results to use it instead of the included TaskResult? (I'm assuming this is the goal of this PR)

django_celery_results/models/__init__.py

diegocastrum · 2022-07-08T09:31:28Z

Well I like the idea and I've some scenarios (multitenant platform) where something like this could help a lot avoiding unnecessary boilerplate code without having to override code from django-celery-result locally. So I like the concept that @jose-padin made.

I think I've some idea to solve the issue you describe @AllexVeldman, I will work on that as soon as I can and share it with all of you to see what do you think about it, but for now I will be off for 3 weeks, so I will do it when I'm back.

Cheers!

hoefling · 2022-08-07T13:24:19Z

@diegocastrum are you still on it or is the task up for grabs?

diegocastrum · 2022-08-09T14:36:52Z

Hi @hoefling, sorry for not replying you earlier. I still working on it and I'm almost done with it. I already have a working scenario in one of my projects. So I expect to push the code very soon, I will try to do it this week. Thanks for asking!

Added a `helpers` module into `models` containing the functions `taskresult_model()` and `groupresult_model()`. * `taskresult_model()`: will try to find the custom model using a dotted path defined under the constant `CELERY_RESULTS_TASKRESULT_MODEL` in the settings of the user's project * `groupresult_model()` will try to do the same using under the constant `CELERY_RESULTS_GROUPRESULT_MODEL`. By default if these attributes are not found `django-celery-results` will use the default models (`models.TaskResult` and `models.GroupResult`). Updated database backend in order to use custom models for `TaskResult and `GroupResult` it they're present. Instead to import explicitely the `TaskResult` and the `GroupResult` (default models from `django-celery-results`) we make use of the model helpers to load the right classes, the custom ones if they're present otherwise we use the default ones. Getting data from `task_kwargs` to extend the `task_properties` and be able to store them into the database (using the custom models). First of all we need a way to get data from `task_kwargs` (or somewhere else) just before a `task_result` record is created, evaluate that data and find the right values that will be used to fill the new fields defined in the custom model. So for this purpose we added a settings module to `django-celery-results` which will hold default settings, the first setting that will contain is a function in charge to get a callback from the settings of the user project. This callback will be feeded by the task `task_kwargs`, which will be intercepted in `DatabaseBackend._get_extended_properties` just before the `task_kwargs` are encoded by `encode_content()` method and send it to the `store_result` method from the object manager of `TaskModel` (Custom/Default one). To end, we must to extend the arguments of the `store_result` method from the `TaskResult` Manager adding `**extra_fields` that will make us able to send extra data to the custom model, when it's defined. --- Resolves celery#305 Fixes celery#314

diegocastrum · 2022-08-10T07:21:53Z

Hi @jose-padin, @AllexVeldman and @hoefling!

I already made a commit improving the initial implementation and adding support to use behind the scenes the custom models defined in the project where django-celery-results is in use, to make use of this feature, the developer only needs define the next 3 constants in his settings module as follow:

Define CELERY_RESULTS_TASKRESULT_MODEL pointing to a custom model for TaskResult
Define CELERY_RESULTS_GROUPRESULT_MODEL pointing to a custom model for GroupResult
Define CELERY_RESULTS_EXTEND_TASK_PROPS_CALLBACK referencing a local function that takes kwargs as argument and returning a dict that will be used to store data into the new fields of the custom model referenced by CELERY_RESULTS_TASKRESULT_MODEL

This implementation is working right now in one of my projects and so far it seems to work fine. Probably we should add some checks, for example:

Some mechanic that forces the user to define the 3 constants instead of just one or two.
Maybe we could make use of the same callback to get parameters from the task task_args, but honestly I don't see the advantage of that right now.

Let me know what you think & Cheers!

AllexVeldman · 2022-08-10T09:34:08Z

@diegocastrum Looks like a viable implementation to me.

Some mechanic that forces the user to define the 3 constants instead of just one or two.

Why is defining just 1 or 2 invalid? from what I can see, having only CELERY_RESULTS_EXTEND_TASK_PROPS_CALLBACK defined could cause weird behaviour.
If it makes sense to define CELERY_RESULTS_TASKRESULT_MODEL without CELERY_RESULTS_EXTEND_TASK_PROPS_CALLBACK I'm not entirely sure about, maybe if you only want to add/override functionality of the class without adding db columns.

AllexVeldman · 2022-08-10T09:38:10Z

django_celery_results/backends/database.py

+            # TODO: We assuming that task protocol 1 could be always in use. :/
+            extended_props.update(
+                extend_task_props_callback(getattr(request, 'kwargs', None)))


Consider calling this from _store_result() so it can be used regardless of the extended result setting.

Nice point! It will be done in the next commit.

AllexVeldman · 2022-08-10T09:40:31Z

django_celery_results/settings.py

+extend_task_props_callback = get_callback_function(
+    "CELERY_RESULTS_EXTEND_TASK_PROPS_CALLBACK"
+)


Consider adding a default callable returning an empty dict so this is an opt-in.
Currently this is mandatory if extended_result=True

Will be done in the next commit

AllexVeldman · 2022-08-10T09:43:52Z

django_celery_results/backends/database.py

@@ -79,6 +79,10 @@ def _get_extended_properties(self, request, traceback):
                # task protocol 1
                task_kwargs = getattr(request, 'kwargs', None)

+            # TODO: We assuming that task protocol 1 could be always in use. :/
+            extended_props.update(
+                extend_task_props_callback(getattr(request, 'kwargs', None)))


Consider passing task_props and request (in _store_result()) to the callback, this gives the developer more context to get whatever additional data it needs to store in the extended model.

That's clear, It will be done in the next commit.

AllexVeldman · 2022-08-10T09:46:59Z

I'm not entirely sure how the docs are managed but I think this warrants a good description with some examples.

AllexVeldman · 2022-08-10T09:51:02Z

django_celery_results/settings.py

+    if callable(callback):
+        return callback
+
+extend_task_props_callback = get_callback_function(


Consider adding some sanity checks on the return value of the callback.
For example that it complies to the Mapping protocol.

Well, get_callback_function is quite generic, and could be used in the future for another purposes, returning different types of data. So far I can see, the sanity checks could be in _store_results() where extend_task_props_callback() is called. What do you think?

I'm personally not a fan of this generic function, I'd err of the side of caution and make the callback handling as clean and explicit as possible so any errors we need to raise have a clear source.
But this is more of a code-style thing so maybe one of the maintainers (@auvipy) can comment.

Good point @AllexVeldman, what do you think @auvipy?

@AllexVeldman & @auvipy I think I found a proper way to control the callback internally being able to check explicitly that return value.

I just created a new function called get_task_props_extension() into the settings module in charge to return an empty dict in case that the CELERY_RESULTS_EXTEND_TASK_PROPS_CALLBACK is undefined and otherwise will check that the return value complies with the Mapping protocol.

Let me know what you think!

`extend_task_props_callback` moved from `_get_extended_properties` to `_store_result`. Suggested by @AllesVeldman. `extend_task_props_callback` will get the `request` object as first parameter and a copy of `task_props` (avoiding potential manipulation of the original `task_props`) as second paramenter. --- Resolves celery#305 Fixes celery#314

auvipy

this will also need a rebase

`get_callback_function()` gets a default callback as an arg returning explicitely an empty dict. `get_callback_function()` raises an `ImproperlyConfigured` exception when the callback is not callable. --- Resolves celery#305 Fixes celery#314

django_celery_results/settings.py

Added `get_task_props_extension` to `settings` module which will raise an `ImproperlyConfigured` when the task_props_extension doesn't complies with the Mapping protocol. `DatabaseBackend` will make use of `get_task_props_extension` to update a potential custom model with the custom properties --- Resolves celery#305 Fixes celery#314

`models` module replaced by a `models` package containing an `abstract` module (for abstract models) and a `generic` module (for the default models, previously in the `models` module) --- Resolves celery#305 Fixes celery#314

… module. Added some minor changes. --- Resolves celery#305 Fixes celery#314

* Fixed import bug

Co-authored-by: Allex <a.veldman@chain-stock.com>

Added a `helpers` module into `models` containing the functions `taskresult_model()` and `groupresult_model()`. * `taskresult_model()`: will try to find the custom model using a dotted path defined under the constant `CELERY_RESULTS_TASKRESULT_MODEL` in the settings of the user's project * `groupresult_model()` will try to do the same using under the constant `CELERY_RESULTS_GROUPRESULT_MODEL`. By default if these attributes are not found `django-celery-results` will use the default models (`models.TaskResult` and `models.GroupResult`). Updated database backend in order to use custom models for `TaskResult and `GroupResult` it they're present. Instead to import explicitely the `TaskResult` and the `GroupResult` (default models from `django-celery-results`) we make use of the model helpers to load the right classes, the custom ones if they're present otherwise we use the default ones. Getting data from `task_kwargs` to extend the `task_properties` and be able to store them into the database (using the custom models). First of all we need a way to get data from `task_kwargs` (or somewhere else) just before a `task_result` record is created, evaluate that data and find the right values that will be used to fill the new fields defined in the custom model. So for this purpose we added a settings module to `django-celery-results` which will hold default settings, the first setting that will contain is a function in charge to get a callback from the settings of the user project. This callback will be feeded by the task `task_kwargs`, which will be intercepted in `DatabaseBackend._get_extended_properties` just before the `task_kwargs` are encoded by `encode_content()` method and send it to the `store_result` method from the object manager of `TaskModel` (Custom/Default one). To end, we must to extend the arguments of the `store_result` method from the `TaskResult` Manager adding `**extra_fields` that will make us able to send extra data to the custom model, when it's defined. --- Resolves celery#305 Fixes celery#314

`extend_task_props_callback` moved from `_get_extended_properties` to `_store_result`. Suggested by @AllesVeldman. `extend_task_props_callback` will get the `request` object as first parameter and a copy of `task_props` (avoiding potential manipulation of the original `task_props`) as second paramenter. --- Resolves celery#305 Fixes celery#314

`get_callback_function()` gets a default callback as an arg returning explicitely an empty dict. `get_callback_function()` raises an `ImproperlyConfigured` exception when the callback is not callable. --- Resolves celery#305 Fixes celery#314

Added `get_task_props_extension` to `settings` module which will raise an `ImproperlyConfigured` when the task_props_extension doesn't complies with the Mapping protocol. `DatabaseBackend` will make use of `get_task_props_extension` to update a potential custom model with the custom properties --- Resolves celery#305 Fixes celery#314

…ttings.

for more information, see https://pre-commit.ci

diegocastrum · 2022-08-17T06:06:40Z

this will also need a rebase

It's done @auvipy

django_celery_results/settings.py

@AllexVeldman

…in `django_celery_results.settings` Fixed a "wrong" description for the `ImproperlyConfigured` exception raised when `task_props_extension` doesn't complies with the Mapping protocol. At this point `task_props_extension` is just a dict that inherits from Mapping, not an explicit instance. Thanks @AllexVeldman Co-authored-by: Allex <a.veldman@chain-stock.com>

diegocastrum · 2022-09-16T07:11:57Z

@auvipy or @AllexVeldman Could you give us an idea about when could be this merged?

AllexVeldman · 2022-09-16T07:38:47Z

Could you give us an idea about when could be this merged?

I'm not involved in merging/releasing, but I think there is still a bit of documentation missing, maybe extend https://github.com/celery/django-celery-results/blob/master/docs/getting_started.rst with an "Extending the results" part or creating a separate page for it.

diegocastrum · 2022-09-16T07:40:48Z

Could you give us an idea about when could be this merged?

I'm not involved in merging/releasing, but I think there is still a bit of documentation missing, maybe extend https://github.com/celery/django-celery-results/blob/master/docs/getting_started.rst with an "Extending the results" part or creating a separate page for it.

You're definetly right. I'll try to do it asap

A new page added to the documentation where we explain how to use this feature. -- issue: celery#305 pull-request: celery#314

docs/extending_task_results.rst

Co-authored-by: Allex <a.veldman@chain-stock.com>

…bstract_models

diegocastrum · 2023-08-26T08:03:00Z

@auvipy Do you see anything else we've to do/work around in order to add this feature into the main branch?

diegocastrum · 2023-09-21T14:46:31Z

@auvipy We don't hear anything from you (here) since Aug 16 2022. Please say something and give us some hope to get this merged.

auvipy · 2023-09-21T14:48:55Z

This is a big change and need more work to get accepted

diegocastrum · 2023-09-21T14:59:23Z

This is a big change and need more work to get accepted

Thanks @auvipy, happy to hear some feedback. Could you give me some more specific clue about where should I put my focus? The last time I dive deep into this was a year ago and since then I used this in another project without having any issue

jose-padin changed the title ~~Issue 305: abstract models~~ Issue #305: abstract models May 16, 2022

jose-padin changed the title ~~Issue #305: abstract models~~ resolve #305 abstract models May 16, 2022

AllexVeldman reviewed Jun 7, 2022

View reviewed changes

django_celery_results/models/__init__.py Outdated Show resolved Hide resolved

AllexVeldman reviewed Aug 10, 2022

View reviewed changes

diegocastrum mentioned this pull request Aug 10, 2022

Consider calling this from _store_result() so it can be used regardless of the extended result setting. #339

Closed

auvipy requested changes Aug 14, 2022

View reviewed changes

auvipy reviewed Aug 16, 2022

View reviewed changes

django_celery_results/settings.py Outdated Show resolved Hide resolved

jose-padin and others added 10 commits August 17, 2022 07:51

[celery#305] Added abstract models.

74c1cc6

`models` module replaced by a `models` package containing an `abstract` module (for abstract models) and a `generic` module (for the default models, previously in the `models` module) --- Resolves celery#305 Fixes celery#314

[celery#305] ChordCounter moved from abstract module to generic…

28a2554

… module. Added some minor changes. --- Resolves celery#305 Fixes celery#314

Issue 305: abstract models

cfe092d

* Fixed import bug

Update django_celery_results/models/__init__.py

7222185

Co-authored-by: Allex <a.veldman@chain-stock.com>

Added newline to the end of

4d2e691

Fixed a NoneType error when the callback is not defined in project se…

948bdf8

…ttings.

diegocastrum force-pushed the devel branch from 0149a54 to 948bdf8 Compare August 17, 2022 06:03

[pre-commit.ci] auto fixes from pre-commit.com hooks

9a5ba40

for more information, see https://pre-commit.ci

AllexVeldman reviewed Aug 17, 2022

View reviewed changes

django_celery_results/settings.py Outdated Show resolved Hide resolved

stumpylog mentioned this pull request Sep 26, 2022

Feature: Transition to celery for background tasks paperless-ngx/paperless-ngx#1648

Merged

14 tasks

diegocastrum added 3 commits October 13, 2022 13:22

[celery#305] Added documentation about this feature.

be25b40

A new page added to the documentation where we explain how to use this feature. -- issue: celery#305 pull-request: celery#314

Merge remote-tracking branch 'origin/devel' into devel

f284747

[celery#305] Fixed some pre-commit failures

fe0c544

AllexVeldman reviewed Oct 14, 2022

View reviewed changes

docs/extending_task_results.rst Outdated Show resolved Hide resolved

docs/extending_task_results.rst Outdated Show resolved Hide resolved

docs/extending_task_results.rst Outdated Show resolved Hide resolved

diegocastrum and others added 3 commits October 17, 2022 19:03

Update docs/extending_task_results.rst

5e02ec1

Co-authored-by: Allex <a.veldman@chain-stock.com>

Update docs/extending_task_results.rst

b47cc0d

Co-authored-by: Allex <a.veldman@chain-stock.com>

Update docs/extending_task_results.rst

09c1b73

Co-authored-by: Allex <a.veldman@chain-stock.com>

AllexVeldman approved these changes Oct 18, 2022

View reviewed changes

diegocastrum added 4 commits November 18, 2022 10:11

Fixed a couple of long lines in docstrings

ba14914

Merge remote-tracking branch 'origin/devel' into devel

8571916

[E275] Added a missing whitespace after keyworkd

350d27b

Merge remote-tracking branch 'upstream/main' into feature/issue-305/a…

a0ff0ea

…bstract_models

auvipy self-requested a review October 3, 2023 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve #305 abstract models #314

resolve #305 abstract models #314

jose-padin commented May 12, 2022 •

edited

AllexVeldman left a comment

diegocastrum commented Jul 8, 2022 •

edited

hoefling commented Aug 7, 2022

diegocastrum commented Aug 9, 2022

diegocastrum commented Aug 10, 2022

AllexVeldman commented Aug 10, 2022

AllexVeldman Aug 10, 2022

diegocastrum Aug 10, 2022 •

edited

AllexVeldman Aug 10, 2022

diegocastrum Aug 11, 2022 •

edited

AllexVeldman Aug 10, 2022

diegocastrum Aug 10, 2022 •

edited

AllexVeldman commented Aug 10, 2022

AllexVeldman Aug 10, 2022

diegocastrum Aug 11, 2022

AllexVeldman Aug 15, 2022

diegocastrum Aug 15, 2022

diegocastrum Aug 17, 2022

AllexVeldman Aug 17, 2022

auvipy left a comment

diegocastrum commented Aug 17, 2022 •

edited

diegocastrum commented Sep 16, 2022

AllexVeldman commented Sep 16, 2022

diegocastrum commented Sep 16, 2022

diegocastrum commented Aug 26, 2023

diegocastrum commented Sep 21, 2023 •

edited

auvipy commented Sep 21, 2023

diegocastrum commented Sep 21, 2023

resolve #305 abstract models #314

Are you sure you want to change the base?

resolve #305 abstract models #314

Conversation

jose-padin commented May 12, 2022 • edited

AllexVeldman left a comment

Choose a reason for hiding this comment

diegocastrum commented Jul 8, 2022 • edited

hoefling commented Aug 7, 2022

diegocastrum commented Aug 9, 2022

diegocastrum commented Aug 10, 2022

AllexVeldman commented Aug 10, 2022

Choose a reason for hiding this comment

diegocastrum Aug 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegocastrum Aug 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegocastrum Aug 10, 2022 • edited

Choose a reason for hiding this comment

AllexVeldman commented Aug 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auvipy left a comment

Choose a reason for hiding this comment

diegocastrum commented Aug 17, 2022 • edited

diegocastrum commented Sep 16, 2022

AllexVeldman commented Sep 16, 2022

diegocastrum commented Sep 16, 2022

diegocastrum commented Aug 26, 2023

diegocastrum commented Sep 21, 2023 • edited

auvipy commented Sep 21, 2023

diegocastrum commented Sep 21, 2023

jose-padin commented May 12, 2022 •

edited

diegocastrum commented Jul 8, 2022 •

edited

diegocastrum Aug 10, 2022 •

edited

diegocastrum Aug 11, 2022 •

edited

diegocastrum Aug 10, 2022 •

edited

diegocastrum commented Aug 17, 2022 •

edited

diegocastrum commented Sep 21, 2023 •

edited