Skip to content
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.

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

3517 - Send reports to discord via celery task #3566

Conversation

nablabits
Copy link
Contributor

@nablabits nablabits commented Jul 13, 2023

It should fix: #3517

In debeb35 we reverted the original implementation of the task as it broke the production environment. The problem was that celery tasks need objects that could be JSON serializable and we were passing a Message instance, so just passing the details fixed the issue.

Screenshot from 2023-07-13 08-04-49

A few things that I noticed:

  • There's no pre-commit version pinned in the requirements.txt so I just used the latest one (3.3.3). In the past I had experienced issues with this so it might be a good idea to pin it.
  • There are a few automated tests, I was about to write them but found no installation of pytest or similar, if you consider so, I'm happy to write them and install the necessary libraries as eventually that will make the codebase more robust.

In debeb35 we reverted the original implementation of the task as it broke the production environment. The problem was that celery tasks need objects that could be JSON serializable and we were passing a `Message` instance, so just passing the details fixed the issue.
@Abeanna
Copy link

Abeanna commented Jul 15, 2023

Ok

Copy link
Collaborator

@melvinebenezer melvinebenezer left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@melvinebenezer melvinebenezer merged commit a637ca6 into LAION-AI:main Jul 24, 2023
3 checks passed
@melvinebenezer
Copy link
Collaborator

@nablabits : we can discuss on the automated tests in discord

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Celery task for Discord notifications of reports
3 participants