Skip to content

TaskAdmin+fields(created_at,updated_at), ideomatic logger, TaskManager TaskManager __str__ try #118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

TaskAdmin+fields(created_at,updated_at), ideomatic logger, TaskManager TaskManager __str__ try #118

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
30261b0
TaskAdmin +readonly_fields "created_at", "updated_at"; fix setup name
ikvk Jan 11, 2022
8831f1d
TaskAdmin +readonly_fields "created_at", "updated_at"; fix setup name…
ikvk Feb 6, 2023
d402b4d
TaskAdmin +readonly_fields "created_at", "updated_at"; fix setup name…
ikvk Feb 6, 2023
0e7a745
del /.idea/
ikvk Apr 26, 2024
01fb783
flake
ikvk Apr 26, 2024
69c482b
logging to idiomatic usage; add logger.exception - Failed to display …
ikvk Apr 27, 2024
e56726c
Update django_dramatiq/admin.py
ikvk Dec 28, 2024
2a01a5b
Merge branch 'master' into minor-improves
ikvk Aug 21, 2025
4ec1563
name="django_dramatiq" back (will make issue)
ikvk Aug 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
/.tox
/.pytest_cache
/.python-version
/.idea/
/benchmark*.svg
/build
/dist
Expand Down
1 change: 1 addition & 0 deletions CONTRIBUTORS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ This file lists the contributors to the `django_dramatiq` project.
| [@OrazioPirataDelloSpazio](https://github.com/OrazioPirataDelloSpazio) | Lorenzo |
| [@timdrijvers](https://github.com/timdrijvers) | Tim Drijvers |
| [@magraeber](https://github.com/magraeber) | magraeber |
| [@ikvk](https://github.com/ikvk) | Vladimir Kaukin |
18 changes: 4 additions & 14 deletions django_dramatiq/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,14 @@
@admin.register(Task)
class TaskAdmin(admin.ModelAdmin):
exclude = ("message_data",)
readonly_fields = ("message_details", "traceback", "status", "queue_name", "actor_name")
list_display = (
"__str__",
"status",
"eta",
"created_at",
"updated_at",
"queue_name",
"actor_name",
)
readonly_fields = ("message_details", "traceback", "status", "queue_name", "actor_name", "created_at", "updated_at")
list_display = ("__str__", "status", "eta", "created_at", "updated_at", "queue_name", "actor_name")
list_filter = ("status", "created_at", "queue_name", "actor_name")
search_fields = ("actor_name",)

def eta(self, instance):
timestamp = (
instance.message.options.get("eta", instance.message.message_timestamp) / 1000
)

"""Estimated time of arrival"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the value of this docstring? It just deciphers a common-known abbreviation, which the function named after.

Copy link
Author

@ikvk ikvk Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First time I took a few moments to decipher it, as I didn't meet her very often. I think it definitely won't hurt

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""Estimated time of arrival"""

I would agree with @amureki on this. Lets remove it before merging.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you serious? How can the documentation line interfere?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I conducted a survey among 10 of my colleagues: "I know exactly what the abbreviation ETA stands for." Most of them are engineers. The result: 40% know for sure.

I didn't think I'd have to defend the docstring either.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more argument:
Explicit is better than implicit.
:D

By the way, is "Estimated time of arrival" the correct decoding of ETA?

timestamp = (instance.message.options.get("eta", instance.message.message_timestamp) / 1000)
# Django expects a timezone-aware datetime if USE_TZ is True, and a naive datetime in localtime otherwise.
tz = timezone.utc if settings.USE_TZ else None
return datetime.fromtimestamp(timestamp, tz=tz)
Expand Down
6 changes: 5 additions & 1 deletion django_dramatiq/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,8 @@ def message(self):
return Message.decode(bytes(self.message_data))

def __str__(self):
return str(self.message)
try:
return str(self.message)
except Exception as e:
return f'Failed to display Task: {e}'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is interesting.

Could you, please, provide some information regarding this?
What kind of exception did you get here and on which conditions?
If this is a valid case, and we'll decide to handle it - then we'd need to document this more and update a test suite to cover it.

Copy link
Author

@ikvk ikvk Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it was a long time ago, the log has already been cleared. I remember that it was for various reasons:

  • When I interfered with the models data.Task during migration
  • Without my participation, maybe it was a bug in dramatiq.
  • On incorrect using dramatiq actor.send, I have not saved example. I think it's somewhere in https://github.com/Bogdanp/dramatiq/issues

If I can find the info, I will definitely let you know. (or the dramatiq project)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you'd happen to find it, I am curious to see. 👍
And yeah, then we can check what'd be the best way to proceed - either in this library, or directly in dramatiq. But without this knowledge, I am afraid, I would not proceed with these lines here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the broken data gets into the database, no matter how, then it actually breaks the admin web page. (we are dealing with bytes in BinaryField)

This edit will avoid 500 errors if the data was suddenly broken. I don't understand your skepticism, security doesn't suffer, performance doesn't suffer either.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, if you felt scepticism in my comments - that was definitely not the intention.

This code might not suffer from security or performance issues, but this is not the point.
We start here, then we end up wrapping every __str__ or __repr__ functions for premature "safety".
But we don't want that, right?
We want to have every line of code meaning something and solving something. And we intend to ensure its function by adding a test case to make sure it won't happen again, and we won't forget about it.

Plus, I think, if we just catch all exceptions here - we'd start ignoring them (as it is kinda handled already, right?), we could accidentally ignore important symptoms leading to this error. So if there is something wrong with bytes in message property - we should handle that bit and not try-catch every place that uses message.

And this is what I am missing here - I do not know the real case that this code change will solve, but it certainly introduces a different behaviour (even though it is a small one).

I hope you could follow me here, and I am sorry, if you still would not agree with my thinking process here, but I truly believe this is only for the good of the project.

Again, I am looking forward to understanding the underlying issue and let's try to solve it in its roots!

Copy link
Author

@ikvk ikvk Apr 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could accidentally ignore important symptoms leading to this error

  • I absolutely agree, I added a message to the log.

The key feature here is that the database cannot guarantee us the encodability of a BinaryField data.
Any byte error will result in 500 at admin web page, no matter how it got into the database.

And I finally found the trace!
I then changed some dramatic settings in the project, but I didn’t log which ones :D

2022-03-10T16:01:44.770354+05:00 job1.stage.int.<mywork>.ru django: Failed to decode message using encoder .
Traceback (most recent call last):
File "/home/int/.pyenv/versions/3.9.7/envs/int-3.9.7/lib/python3.9/site-packages/dramatiq/message.py", line 102, in decode
return cls(**global_encoder.decode(data))
_pickle.UnpicklingError: invalid load key, '_'.

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "/home/int/.pyenv/versions/3.9.7/envs/int-3.9.7/lib/python3.9/site-packages/dramatiq/brokers/rabbitmq.py", line 511, in __next__
message = Message.decode(body)
File "/home/int/.pyenv/versions/3.9.7/envs/int-3.9.7/lib/python3.9/site-packages/dramatiq/message.py", line 104, in decode
raise DecodeError("Failed to decode message.", data, e) from e
dramatiq.errors.DecodeError: Failed to decode message.


2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def rel(*xs):


setup(
name="django_dramatiq",
name="django-dramatiq",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Any reason to change the name of the project?? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes,
Let github.com to show "Used by" block at main repo page (setup.name, on my own experience):
For my lib it works, I found this decision somewhere at web.
ikvk/imap_tools@f1b2614#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7R16

Here is rules:
https://packaging.python.org/en/latest/specifications/name-normalization/

This is not killer fature, but useful.

If I saw such a MR for my library, I would also double-check everything 100 times.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let github.com to show "Used by" block at main repo page

I dont think this is the case, and its most likely due to the number of public repo dependents being less than 100..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe so, I suggest you try, and if anything, you can roll back the changes in the minor release immediately after the release.
In my old screenshot, the number of uses was 500, there is a chance of success.
*The block is not always visible immediately after opening the repository page, sometimes you need to wait a few seconds and refresh the page.

version=version,
description="A Django app for Dramatiq.",
long_description="Visit https://github.com/Bogdanp/django_dramatiq for more information.",
Expand Down