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.

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

Jump to bottom

Implement Celery task for Discord notifications of reports #3517

Closed
olliestanley opened this issue Jun 25, 2023 · 3 comments · Fixed by #3566
Closed

Implement Celery task for Discord notifications of reports #3517

olliestanley opened this issue Jun 25, 2023 · 3 comments · Fixed by #3566
Labels
backend

Comments

@olliestanley
Copy link
Collaborator

The previous implementation caused a bug and so is reverted by #3516
@nablabits
Copy link
Contributor

Hi @olliestanley , I'm happy to give this a try. I've managed to get the dev server running locally and spotted where that button lives and what are the methods called. I have a couple of questions:

  • Did you revert the previous implementation as a quick fix or rather because it was a dead end? I say so because if it was a quick fix I can reinstate it and debug it.
  • If it was a dead end, do you have in the code base some similar task I can take a look at as a guide?
  • Finally, do you have a discord channel sandbox that I can use to test the messages arriving? Alternatively I can set up something on my own

Chances are this issue might have a steep learning curve, so if anyone more knowledgeable is keen to pick this up, feel free, I'm happy with the learning experience anyways.

@olliestanley
Copy link
Collaborator Author

Hi @olliestanley , I'm happy to give this a try. I've managed to get the dev server running locally and spotted where that button lives and what are the methods called. I have a couple of questions:

  • Did you revert the previous implementation as a quick fix or rather because it was a dead end? I say so because if it was a quick fix I can reinstate it and debug it.
  • If it was a dead end, do you have in the code base some similar task I can take a look at as a guide?
  • Finally, do you have a discord channel sandbox that I can use to test the messages arriving? Alternatively I can set up something on my own

Chances are this issue might have a steep learning curve, so if anyone more knowledgeable is keen to pick this up, feel free, I'm happy with the learning experience anyways.

I think the previous approach was fine in principle, but there was a bug causing the report function to be broken entirely, and we wanted to fix prod ASAP so just reverted the change.

We don't have a sandbox, you might have to set something up for that unfortunately.

Hopefully this should be a fairly small change so would be great if you could take a look!

@nablabits
Copy link
Contributor

Cool, thanks for the answers, I will work through this these days and will let you know if I get stuck 🤞

@nablabits nablabits mentioned this issue Jul 13, 2023
Merged
melvinebenezer pushed a commit that referenced this issue Jul 24, 2023
@nablabits
3517 - Send reports to discord via celery task (#3566)
a637ca6 
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](https://github.com/LAION-AI/Open-Assistant/assets/33068707/1e76d71f-7182-49ec-8e94-38ef2fce56a2)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
backend
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

3517 - Send reports to discord via celery task nablabits/Open-Assistant
2 participants
@olliestanley @nablabits