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

Add CommandTree.get_hash #10082

Closed
wants to merge 1 commit into from
Closed

Conversation

mikeshardmind
Copy link
Contributor

Summary

Adds a method to get a hash of the command tree payload as it would be sent to discord when synced.

This is intended to allow users to sync only when needed programmatically (by comparing with a stored prior hash), as well as to potentially enable some level of automatic syncing to be done by the library on behalf of users in the future built on top of this.

Details about a potential API to automatically sync global commands based on this info here: https://discord.com/channels/336642139381301249/1326219331050147940

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

discord/app_commands/tree.py Outdated Show resolved Hide resolved
discord/app_commands/tree.py Show resolved Hide resolved
@Rapptz
Copy link
Owner

Rapptz commented Jan 19, 2025

I'm not really sure this belongs in the library proper? Part of the problem is that (unfortunately) a lot of people do a lot of extra non-immutable work within their translators that can be potentially expensive (for some God forsaken reason, people actually do HTTP requests here) and done multiple times in this scenario.

For example if someone wanted to store a hash and then do the call to compare, they'd end up calling all the translation machinery once to generate the hash and then again once they find out it's different. This feels subpar compared to just calling it once when you know it's changed (i.e. as part of a scheduled translation routine or whatever).

We could make the argument that cases like these are uncommon, and maybe that's agreeable but I'm not really sure this machinery is worth it for the people who want to avoid a sync call. Truth be told the sync rate limit is not that bad, since it only triggers on new commands added per day. That is to say that updates don't suffer from as much of a rate limit, which is the main use case for a hash to detect anyway. If you wanted to be more careful of the rate limit you're probably better off making a mapping with the full name of every command and then checking if those sets are different rather than a hashing scheme.

@mikeshardmind
Copy link
Contributor Author

mikeshardmind commented Jan 19, 2025

Part of the problem is that (unfortunately) a lot of people do a lot of extra non-immutable work within their translators that can be potentially expensive (for some God forsaken reason, people actually do HTTP requests here) and done multiple times in this scenario.

😐 I was unaware there's anyone doing something like this, from my perspective, translations should basically be static content (Technically, basically a state machine with constant time performance wrt things like numbers, though this shouldn't be part of the stuff which is synced), and I even thought it was odd that the translation machinery is async.

The only current issue with this existing outside of the library is that whoever is implementing it needs to track which things require syncing and how that may change over time. While this isn't impossible, there's a lot hidden from users by default about this, especially around the translation machinery. If leaning on how discord.py does it internally, then there's a reliance on things which aren't guaranteed. This actually changed at some point, as I remember having to change something in the implementation of this in two of my bots (one private, one public currently)

Maybe there's a way to get this returned as part of sync, and for sync to take a "last known hash" parameter that if it matches, skip syncing to prevent double calling? But then that changes the return type of that and would be breaking.

@mikeshardmind
Copy link
Contributor Author

mikeshardmind commented Jan 19, 2025

I've pushed changes that handle the potential non-determinism of command ordering + the other stuff mentioned, but that's not necessarily going to be relevant if it's not suitable for inclusion.

I'll think about whether there's a solid way to prevent double-calling the translation machinery and other options that might address the same issues reasonably, but things like people making HTTP requests in translations leaves the door open to the translation being subtly different each time anyhow, and this is part of what is synced.

@mikeshardmind
Copy link
Contributor Author

mikeshardmind commented Jan 19, 2025

I think there's reasonable value to this still, but I can't come up with an answer that accounts for the double work here that I think helps the people who would benefit from it without a breaking change. The best I've been able to come up with involves some callbacks that get invoked during the sync flow and can preempt going through with the sync, as this gives a non-breaking point where no work would be duplicated, but I'm not sure this is the most appropriate way for the users this was intended to help.

The end goal here was essentially that for someone just running this locally or on a single server and without a complexly orchestrated larger bot, any changes they made could result in automatically syncing when they start the bot, without syncing on start unconditionally, and without needing to know themselves which things required syncing. (for example, they fix the description of a command: that requires a sync, but autocomplete changes don't. Or a translation file gets updated, this may or may not need a sync depending on which translation was updated, and where it is displayed in discord.)

While the ratelimit for sync appears reasonable at first, we've had people who sync unconditionally in setup_hook get ratelimited in help channels, with indications that this is dynamic in some way, and not just on additions.

@mikeshardmind
Copy link
Contributor Author

dropping this for now, but may revisit the idea for the 2.6 series, there's some questions about ergonomics that would be worth exploring before committing to something intended to assist users here. In the meantime, users interested in this can continue syncing manually when changing anything which displays when typing / in discord, do something like what's present here: https://github.com/mikeshardmind/salamander-reloaded/blob/5470abb623e08722df0081ef188ebe79b43bae23/src/salamander/bot.py#L63-L72 with the knowledge that they are currently using something which may change, and need to check at each version bump of discord.py, or automate their own check of when their commands change in terms of synced signature.

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.

3 participants