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

Replace all relevant Interaction.send/followups with send_discord_message wrapper #59

Merged
merged 13 commits into from
Dec 18, 2024

Conversation

alexzhang13
Copy link
Collaborator

@alexzhang13 alexzhang13 commented Dec 17, 2024

Description

To avoid issues where Discord complains about the correct usage of send / followup, we add a wrapper that checks which is valid, and uses the correct command. This feature is important for callbacks, where an existing response might exist, despite the GitHub slash command function assuming it does not.

Checklist

Before submitting this PR, ensure the following steps have been completed:

  • Run the slash command /verifyruns on your own server.
    • Run the cluster bot on your server:
      python discord-bot.py
    • Start training runs with the slash command /verifyruns.
    • Verify that the bot eventually responds with:
      ✅ All runs completed successfully!
      
      (It may take a few minutes for all runs to finish. In particular, the GitHub
      runs may take a little longer. The Modal run is typically quick.)
      For more information on running a cluster bot on your own server, see
      README.md.

"Error in leaderboard creation.",
ephemeral=True,
)


@discord.app_commands.describe(leaderboard_name="Name of the leaderboard")
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alexzhang13 I think we should just make it use our send_discord_message in all cases, so would replace these interaction.response.send_message with it as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh there's a weird case with embed=embed or whatever where it doesn't exactly work, but otherwise I can replace the rest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the issue with embed=embed? Does one of the followup.send or interaction.response.send_message not support it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah they expect a message, but I can change this as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can work around this in a later PR though, for now I think we know what to expect for the embeds (it's always a followup) so we can keep it as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd replace all occuranges of the discord default way with our wrapper, sadly I cannot mark those as Github doesn't allow comments on lines outside of PR, but in methods submit_modal and get_leaderboard_submission

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also in here, would change the occurances, just so we're doing it all the same way

@alexzhang13 alexzhang13 requested a review from S1ro1 December 18, 2024 00:06
Copy link
Collaborator

@S1ro1 S1ro1 left a comment

Choose a reason for hiding this comment

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

lgtm

@S1ro1 S1ro1 merged commit 05363e7 into main Dec 18, 2024
1 check passed
@S1ro1 S1ro1 deleted the add_send_discord_message branch December 18, 2024 13:43
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.

2 participants