anoadragon453 / busty Goto Github PK

Running !list on spotty internet, the download can time out and give an unhandled TimeoutError. We may even want to notify the user in Discord that some files failed to download when the list appears

Interestingly, Nextcord documentation claims that Attachment.save() will only raise either nextcord.HttpException or nextcord.NotFound, so this might be case of lacking documentation on their part as well.

Task exception was never retrieved
future: <Task finished name='Task-45' coro=<scrape_channel_media.<locals>.dl_file() done, defined at /home/jack/Repos/busty/main
Traceback (most recent call last):
  File "/home/jack/Repos/busty/main.py", line 722, in dl_file
    await attachment.save(attachment_filepath)
  File "/home/jack/Repos/busty/env/lib/python3.10/site-packages/nextcord/message.py", line 225, in save
    data = await self.read(use_cached=use_cached)
  File "/home/jack/Repos/busty/env/lib/python3.10/site-packages/nextcord/message.py", line 267, in read
    data = await self._http.get_from_cdn(url)
  File "/home/jack/Repos/busty/env/lib/python3.10/site-packages/nextcord/http.py", line 359, in get_from_cdn
    return await resp.read()
  File "/home/jack/Repos/busty/env/lib/python3.10/site-packages/aiohttp/client_reqrep.py", line 1036, in read
    self._body = await self.content.read()
  File "/home/jack/Repos/busty/env/lib/python3.10/site-packages/aiohttp/streams.py", line 375, in read
    block = await self.readany()
  File "/home/jack/Repos/busty/env/lib/python3.10/site-packages/aiohttp/streams.py", line 397, in readany
    await self._wait("readany")
  File "/home/jack/Repos/busty/env/lib/python3.10/site-packages/aiohttp/streams.py", line 303, in _wait
    with self._timer:
  File "/home/jack/Repos/busty/env/lib/python3.10/site-packages/aiohttp/helpers.py", line 721, in __exit__
    raise asyncio.TimeoutError from None
asyncio.exceptions.TimeoutError

Right now Busty's main.py is a monolithic 900+ lines of code, with many only somewhat related parts being interspersed with each other. We should split related functions into separate modules for better organization

We have the following code for detecting whether an attachment in a Discord message is a song/video:

However, on accidentally running !list on a fairly old channel with a lot of messages/attachment, I got the following stack trace:

 % python ./main.py 
We have logged in as Busty Dev#3334.
Ignoring exception in on_message
Traceback (most recent call last):
  File "/home/user/code/busty/env/lib/python3.10/site-packages/discord/client.py", line 343, in _run_event
    await coro(*args, **kwargs)
  File "/home/user/code/busty/./main.py", line 57, in on_message
    await list(message)
  File "/home/user/code/busty/./main.py", line 197, in list
    channel_media_attachments = await scrape_channel_media(message.channel)
  File "/home/user/code/busty/./main.py", line 232, in scrape_channel_media
    not attachment.content_type.startswith("audio")
AttributeError: 'NoneType' object has no attribute 'startswith'

It turns out that Attachment.content_type can be None, according to discord.py's documentation. Since it's new in discord.py v1.7, my hunch is that Discord only started recording and returning the media type of attachments recently, and thus this field is None when querying older message attachments.

This doesn't really affect Busty's at all, as we're always dealing with newly uploaded media files, but it'd be good hygiene to handle, especially if my hunch is wrong.

Currently if a song fails midway through playback, we'll just log an exception and carry on to the next song.

I think we could handle this in a slightly better way. Namely:

• Try replaying the song (though be careful of falling into an infinite loop).
• Send a message in the room that playback failed, rather than having no feedback on Discord. Otherwise everyone but the author will just think that the song simply ended abruptly.

Not sure if this should be an enhancement or a bug. Cover art works for MP3 and FLAC but does not seem to for other formats like WAV, OGG, and OPUS. This is likely a limitation of TinyTag itself, from the README: "Additionally you can also get cover images from ID3 tags". Though this may not be 100% correct, since as far as I know FLAC does not use ID3 tags but WAV does. Either way, after looking through the TinyTag code and methods it seems not possible to do for OGG with Vorbis comments/tags.

At the very least mutagen can do this, and it seems to be one of the most mature and robust media tagging libraries. It's certainly "heavier" than TinyTag (though still has no dependencies except the Python stl), but if we're aiming for maximum compatibility I think it's a worthy sacrifice. Also, it seems to be able to detect image formats itself so it would remove the Pillow dependency.

I checked, and songs with OGG cover art have already been submitted for Busty's 3 in five days (don't worry, I didn't listen to them), so I think it's a good idea to add this feature before then.

Not certain if this is a bug, a missing feature, or an oversight but it should be implemented. It's the third in the trio of Busty's shortcomings revealed during Busty's 3 (along with #70 and #73). Busty only responds to !stop when a song is curently playing.

!bust <song #> failed during the most recent busty's.
The playlist had 39 songs. Snippet for context:

Fail case:

Some users have complained that Busty's audio clips when she is set to 100% volume on Discord. This is probably an upstream Discord bug, and seems to occur on Windows but not Linux. To mitigate this we should turn Busty's output down 10% or so.

The current state of affairs:

This message was a bit difficult to see amongst the rest of everyone's messages. Ideally we'd make use of Discord's rich message system, which provides some padding around messages, and makes them look more official. Plus we have more options for formatting.

#8 added a feature where the bot would change their name to that of the currently playing song. When all songs had finished playing, or an admin calls !stop, the bot should change it's name back to what it was called originally.

While that did seem to work in testing that PR at the time, the functionality seems to have broken along the way. The bot's name does not change after the final song finishes playing, nor when an admin calls !stop, leaving the bot's name as that of the last played song.

Temporarily pin the "now playing" message for the duration of the song (and unpin it afterwards) so that people can check channel pins and see the cover art and More Info easily at any point. Right now the Now Playing message can be buried quickly when chat is very active.

So that admins can test a !list before doing it publicly.

As of now Busty does not delete any attachment files, leaving them to accumulate. In theory, if run without maintenance indefinitely she would eat up more and more drive space even though once a bust is done she won't access any files in the attachments folder without re-downloading. I think Busty should keep track of which files she has created each bust and then delete them all afterwards.

I'm against an implementation which blindly deletes ALL files in the attachments folder after each bust. We should be careful to remove only files we have created.

I have found an issue:

• When audio is playing and if !list command is used, it shows the list of all audios scraped. But if !skip is used after list command, it starts playing audio from the top of the list rather than continuing from the present index.

Here are some screenshots:

2 hours of continuous busting with no breaks can wear on our stamina. Busty should calculate the halfway point of the bust and put a 10 minute or so intermission in there.

Such that people can easily check which song is currently playing. Otherwise they need to check the pinned message and try and keep the current or previous song in mind.

We only have 32 characters to play with, so song names may be truncated. If this happens, we should end the title with an ellipsis character: …

It would be fun to place an emoji in front as well. Perhaps a random emoji (excluding some not as fun ones, like the flags) that switches per artist.

Instead of hard-coding the Discord bot token, the user should pass it via an environment variable when running the program. As an example:

BUSTY_DISCORD_ACCESS_TOKEN=xxx python main.py

The same could be done for settings. If a specific setting isn't provided, then a sensible default value (what the busty's server uses really) should be used instead. For example:

BUSTY_DISCORD_ACCESS_TOKEN=xxx BUSTY_SECONDS_BETWEEN_SONGS=15 python main.py

Prefixing with BUSTY_ ensures that the environment variable names do not collide with any other programs.

I advocate for using environment variables instead of a settings file, as:

1. It's quicker to set environment variables than copying a sample config file and the editing values.
2. It's more compatible with containers.

For cons vs. a config file:

1. It can be messier if we end up having tons of options.

We tend to run !list more than once per bust, to do things like get Google Form info, make sure everything looks good, etc. We also run it a lot locally when testing. On a large number of songs there can be significant delay and bandwidth consumption from downloading all of the files. It would be nice to have a feature which allows songs to be "cached" so that runs of !list occurring close in time don't both need to download everything.

Despite my feature proposal being sort of long, this wouldn't actually be all that complicated to code. It just involves a lot of arbitrary choices for which I'd like to make recommendations, and I want to hear what others think before implementing it.

More concrete feature proposal:

• Save file <discord_filename> as <discord_message_id>.<attachment_num>.<discord_fileext>
  • We need <attachment_num> because discord messages can have multiple attachments
  • Discord message ids are universally unique
  • Previously we needed unique file ids per-channel and chose <index>.<discord_filename>
    • Now we need them globally unique, someone could delete a message with an old version of their song and resend it for example
    • Our current choice actually has a bug! Discord seems to accept 255-length filenames which is is the max filename length on most common filesystems (more or less) so we'd need to truncate very long filenames, ruining our claim to perfect archival through filenames anyways
• On each !list:
  1. When scraping channel for files, first check to see if cached version exists in attachments/ (as <message_id>.<file_ext>)
  2. If it doesn't exist, download it
  3. Set the modified time of the file to now
  4. Keep only the max(X, # of songs in current !list) newest files and delete all others, where X is say 50?
  • I previously argued that I was against indiscriminately deleting files in the attachment folder mainly because "delete all files in X directory" is never a piece of code you want to have a bug with. But I think in this case it's warranted.
  • We could also do something like "delete all files older than 24 hours", but I don't like this because then we can't prerun !list at any point during the two weeks and it stays cached
  • We could also do something like "delete the oldest files until total filesize is under a certain amount OR only bust files remain" but unless we're extremely space limited this seems like overkill

Given the fact that the Busty bot's profile picture has been updated and the reasons we updated it, we should change the promotional images to ones which use the new profile picture.

Since discord.py is discontinued, we should probably switch to a maintained fork. The two to choose from seem to be nextcord and pycord. I'm in favor of pycord since It seems to have a lot more attention and active development from the Github.

pycord is in theory a drop-in replacement (we'd only need to change requirements.txt) and nextcord would be almost drop in (the import changes to "nextcord"). @anoadragon453 If you are okay with switching to pycord I can make sure all our code runs fine with it and submit the pull request.

Attempting to !bust while busting should result in Busty saying something along the lines of "we're already bustin', sugar".

Currently we just hit this exception as the command silently fails:

Busty currently changes her name to the currently playing song, so that it's easy to see at a glance.

As an additional nice-to-have, we could also bold the title of the currently playing song in the song listing message. Like so:

This would require editing the song list message when a song starts playing. We could factor out the code that generates the contents of the !list command into a new function. This function would take the index of the currently playing song, and bold that song's filename in the output.

Most audio formats allow for storing tag info like Title and Artist. It would be nice if Busty could be made aware of this info to display in the !list and "Now Playing" messages, instead of just taking it from filenames.

mp3 tags are really all we need, but tag support for flac, wav, ogg, etc is a good idea also if it's not too much extra work.

If you send a txt file as an MP3 Busty will just silently skip it on playback. Instead, she should send an error message.

Discovered in #117 (review)

What license should we use? I'm not sure it matters all that much, but it's good practice to have one.

https://choosealicense.com/licenses/

I'd probably opt for Apache myself, as it has slightly more commercial limitations than MIT.

This mp3 file causes busty to hang, requiring a restart to regain functionality.

No logs are printed.

I would start by running busty with a debugger attached and seeing where the interpreter is when the hang occurs.

There can be a significant delay between when !list is run and when the list actually shows up. Busty should react with a thumbs up to !list commands as immediate feedback that the command is being actually processed.

Busty's uses a lot of Optional global variables which are only not None when the bust is active or items are listed. This makes static type checking very difficult, doesn't couple related components very well, and is just plain ugly. We should put all of these bust related variables into a BustController class, which can better keep track of bust state.

Due to python's nature of ducktyping (lack of explicitly marking variables with strong types), it's possible to make small errors while programming that result in real exceptions. One example is #82, where it was assumed that message.author was always a Member. But in reality, it could either be a Member or a User.

Tools such as mypy can help catch these errors before they're discovered in production, by checking types and raising errors if a given operation (such as User.roles) is invalid.

Personally, I'm very familiar with using mypy as part of my projects, and find it an essential tool for writing maintainable python code. The downside of including it is that we need to ensure we add type hints where variables are ambiguous. However, I consider this more of a necessary investment than a negative point.

To test it out, simply do:

pip install mypy
mypy main.py

On current main, it will produce the following output:

(venv) [user@izzy busty]$ mypy main.py 
main.py:9: error: Skipping analyzing "mutagen": module is installed, but missing library stubs or py.typed marker
main.py:9: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
main.py:10: error: Skipping analyzing "mutagen.flac": module is installed, but missing library stubs or py.typed marker
main.py:11: error: Skipping analyzing "mutagen.id3": module is installed, but missing library stubs or py.typed marker
main.py:12: error: Skipping analyzing "mutagen.ogg": module is installed, but missing library stubs or py.typed marker
main.py:94: error: Item "User" of "Union[User, Member]" has no attribute "roles"
main.py:309: error: Item "None" of "Optional[Guild]" has no attribute "voice_channels"
main.py:309: error: Incompatible types in assignment (expression has type "Union[List[VoiceChannel], Any]", variable has type "List[Union[VoiceChannel, StageChannel]]")
main.py:310: error: Item "None" of "Optional[Guild]" has no attribute "stage_channels"
main.py:319: error: Item "None" of "Optional[Guild]" has no attribute "get_member"
main.py:319: error: Item "None" of "Optional[ClientUser]" has no attribute "id"
main.py:335: error: Item "None" of "Union[Member, None, Any]" has no attribute "edit"
main.py:349: error: Item "None" of "Union[Member, None, Any]" has no attribute "display_name"
main.py:482: error: Argument 1 to "scrape_channel_media" has incompatible type "Union[Union[TextChannel, Thread, DMChannel, PartialMessageable], GroupChannel]"; expected "TextChannel"
main.py:494: error: Need type annotation for "embed_description_list" (hint: "embed_description_list: List[<type>] = ...")
main.py:573: error: Incompatible types in assignment (expression has type "Union[Union[TextChannel, Thread, DMChannel, PartialMessageable], GroupChannel]", variable has type "Optional[TextChannel]")
main.py:607: error: Argument 1 to "save" of "Attachment" has incompatible type "str"; expected "Union[BufferedIOBase, PathLike[Any]]"
Found 16 errors in 1 file (checked 1 source file)

Note main.py:94: error: Item "User" of "Union[User, Member]" has no attribute "roles", which would've prevented #82.

If this sounds reasonable, I'll put up a PR to add in the necessary CI, deps and fixes.

To ease our worries about people messaging and going over cap. Add a command !count with optional parameter #channel which prints the number of messages in a channel, or "{MAXIMUM_MESSAGES_TO_SCAN}+" if the amount is more than MAXIMUM_MESSAGES_TO_SCAN

It would be convenient if Busty could automatically generate Google Forms for song voting.

From a bit of research, it seems the standard way to do this is to use Google Apps Script. Google Apps Script code is only run remotely on Google Servers and not locally, however scripts can be created and deployed locally through the REST API.

I plan on adding this feature myself, hopefully starting within the next week or so.

Add an additional long-form response question to the generated apps script code for questions/comments/suggestions.

Testing with a Python 3.10 system that doesn't have libopus installed (and thus is unable to play tracks) should raise a nextcord.opus.OpusNotLoaded Exception upon attempting to play a song. What I noticed after #78 merged is that this exception is still raised, but does not get printed until the process is killed:

$ BUSTY_DISCORD_TOKEN=<redacted> python ./main.py 
We have logged in as Busty Dev#1234.
^CTask exception was never retrieved  # notice the ^C here indicating when I sent a SIGINT (ctrl-C)
future: <Task finished name='Task-51' coro=<play_next_song() done, defined at /home/user/code/busty/./main.py:511> exception=OpusNotLoaded()>
Traceback (most recent call last):
  File "/home/user/code/busty/./main.py", line 581, in play_next_song
    active_voice_client.play(
  File "/nix/store/4g63yzrbr75r95ks0289f3gf0hl42mhv-python3-3.10.5-env/lib/python3.10/site-packages/nextcord/voice_client.py", line 607, in play
    self.encoder = opus.Encoder()
  File "/nix/store/4g63yzrbr75r95ks0289f3gf0hl42mhv-python3-3.10.5-env/lib/python3.10/site-packages/nextcord/opus.py", line 365, in __init__
    _OpusStruct.get_opus_version()
  File "/nix/store/4g63yzrbr75r95ks0289f3gf0hl42mhv-python3-3.10.5-env/lib/python3.10/site-packages/nextcord/opus.py", line 358, in get_opus_version
    raise OpusNotLoaded()
nextcord.opus.OpusNotLoaded

Reverting #78 locally causes the Exception to be printed immediately.

I did come across python/cpython@7ce1c6f, however I think this is a red herring, as !stop, and thus .cancel(), has not been called on the task. Good to keep in mind for the future however.

I'm not thrilled that it's hiding exceptions until the process is killed (potentially after other logging occurs, confusing the log timeline).

Specifically this message:

So that we don't need to do so manually.

If the busty process is sent a signal to close during a bust, she will leave behind files in the attachments folder that need to be cleaned out manually, as well as keep whatever nickname she had at the time. We should use the atexit module to attempt to revert Busty's nickname and delete files she is tracking before the program ends.

EDIT: We can't use the atexit module, as the discord client event loop is closed before atexit runs. We would probably have to reset the nick through some nextcord event hook.

#25 added the ability to read media information from files. #3 made it so that Busty changes her name to that of a song's artist and name before it plays.

We should update the Busty name change feature so that it pulls the media information if available.

Since we have limited characters to work with in a Discord displayname (up to 32), I suggest that we use the form:

{random_emoji}{file_artist_name or discord_displayname} - {file_song_name or cleaned_up_file_name}

If two people submit songs with the same filename, the latter will overwrite the former in the media directory. We should save files with local filenames that don't conflict, perhaps something like my_song.mp3 --> <discord message id>.mp3.

nextcord's put out another alpha release with some bug fixes and new features: https://github.com/nextcord/nextcord/releases/tag/2.0.0a7

We should consider upgrading (and testing that everything still works).

Many audio formats allow for embedded cover art, and Discord embeds have a special image field. It would be great if Busty could parse this cover art tag and display it during the "Now Playing" embed

Helpeful Links:
Parsing image tags with TinyTag.
Using local image files for Discord embeds.

So that people who want to archive the songs can do so easily.

We wouldn't be able to host the songs on Discord, but if we end up integrating with google services as part of #12, then uploading to google drive may be an option.

Perhaps either attaching it as a link to the !list command output, or sending it as a message at the end of a bust.

A request from Toasterless.

Especially with how long busts are getting, it might be a good idea to add a !pause and !resume command so we can implement a break

It would be nice if Busty could DM the user a preview of what their song will look like on the "Now Playing" message upon submit, since some people might be unsure they're doing it right especially for more exotic (but space efficient!) formats like ogg or opus. It would also serve the dual purpose of a "confirmation", though ofc even if busty is not running when a song is submitted she will still play it during bust time.

This is the first feature we'd implement which would require Busty to be "always on" instead of just on during the sync. Because of this, there should be some sort of flag or environment config allowing you to turn off all "always on" features.

TinyTag fails to parse videos, such as this example, resulting in the folllowing stacktrace:

Ignoring exception in on_message
Traceback (most recent call last):
  File "/home/user/code/busty/env/lib/python3.10/site-packages/nextcord/client.py", line 415, in _run_event
    await coro(*args, **kwargs)
  File "/home/user/code/busty/main.py", line 90, in on_message
    await command_list(message)
  File "/home/user/code/busty/main.py", line 396, in command_list
    song_format(local_filepath, attachment.filename),
  File "/home/user/code/busty/main.py", line 141, in song_format
    tags = TinyTag.get(local_filepath)
  File "/home/user/code/busty/env/lib/python3.10/site-packages/tinytag/tinytag.py", line 189, in get
    parser_class = cls.get_parser_class(filename, af)
  File "/home/user/code/busty/env/lib/python3.10/site-packages/tinytag/tinytag.py", line 173, in get_parser_class
    raise TinyTagException('No tag reader found to support filetype! %s' % (filename,))
tinytag.tinytag.TinyTagException: No tag reader found to support filetype! attachments/002.2021-04-24_02-19-59.mkv

We should try/except attempting to read audio tags from a file, and give up if an exception is raised.

We wouldn't want to let people know how long the stream is going to be from the start, because then it lets you guess the lengths of the last songs. However it's cool to know much content was created overall at the end.

Just in case something goes wrong halfway through, and we need to restart without typing !skip 15 times.

@Cephian and I were wondering whether one could place lots of spaces/newlines in their artist/title tag in order to break the output of the bot (make it spam many messages to the channel as it tries to print a giant !list output).

This could apply even to tags that aren't fully whitespace - just put a non-whitespace character in and you'll pass the existing is_valid_tag check.

Perhaps we should apply a sane total limit to the artist/title (500 characters?) and strip newlines altogether.

The output of the !list command makes use of markdown in its rendering. On desktop/web and iOS platforms, this markdown is rendered correctly. On Discord Android however, markdown does not look to be rendered, and output looks like the following:

whereas it should look more akin to:

@Cephian has reported the bug to Discord. There's not much we can do on our side unless we modify the output of the command to no longer require markdown.

Unfortunately, looking around, this bug looks to have been known as early as 2019.

The maximum amount of characters in a Discord field value is 1024 (source) but the Discord message character limit is 2000. Submitting a song with a "More Info" section over 1024 characters causes Busty to freeze/crash when that song comes up to play.

I propose we just truncate the message to 1024 characters.

While the command reference in the README says what each command does individually, the README lacks a bird's-eye view of Busty's intended purpose. As it is, it would be easy to at first mistake Busty for being an underfeatured general music bot.

Which would allow developers to test >8MB songs without needing to pay for Discord nitro.

As any old file could be behind a cdn.discordapp.com link, checking for known audio/video file extensions in the link before downloading the file would be necessary.