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

Commands to delete excess backup videos and reorder versions #1398

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

susanodd
Copy link
Collaborator

@susanodd susanodd commented Nov 26, 2024

Goal: from a selected set of GlossVideo objects in the admin, delete the video file and the object if the version is a backup.
Reorder the remaining backup videos for the glosses in the set.

@vanlummelhuizen if you're working today on Signbank, the new "video admin" command merely prints what it is doing, it does not actually change anything.
I added a rename to it to get the correct video file extension on the backup file.

Perhaps the command is a bit weird now. Or perhaps it should be split into multiple commands.

The command needs to operate on a selection of GlossVideo objects.
So it does what it's supposed to (although commented out and doesn't), and for the "remaining" objects that were not deleted, it renames them and fixes the version number.

Since the command currently "doesn't do anything other than print stuff" it is harmless to try on the real server.

after selection was deleted, version numbers adjusted for new set. Change extension of backup file if it has the wrong video extension for its format.
@vanlummelhuizen
Copy link
Collaborator

@susanodd Thank you for this PR and making it not too big. I can see it does not do anything, but I assume it should do something in the future. I guess you first want to see that it is outputting the correct stuff before letting it (re)move and rename files.

Although the PR itself is not big, the functions you wrote are a bit long and sometimes hard to follow. I think there are several places where you could improve this. In function remove_backups:

  • You can get a distinct set of glosses with distinct_glosses = Gloss.objects.filter(glossvideo__in=queryset).distinct() instead of
    glosses_in_queryset = [obj.gloss for obj in queryset]
    distinct_glosses = list(set(glosses_in_queryset))

Also, move that statement to the place where it is used.

  • You could get rid of the if obj.version == 0: clause if you do queryset = queryset.filter(version__gt=0).

  • This code:

    lookup_backup_files = dict()
    for gloss in distinct_glosses:
        lookup_backup_files[gloss] = GlossVideo.objects.filter(gloss=gloss, version__gt=0).order_by('version', 'id')
    for gloss, videos in lookup_backup_files.items():

could be reduced to

    for gloss in distinct_glosses:
        videos = GlossVideo.objects.filter(gloss=gloss, version__gt=0).order_by('version', 'id')
  • There are ways of reducing the if DEBUG_VIDEOS: clauses to one line if there is only a print statement. E.g something like print('bla') if DEBUG_VIDEOS else None.

  • Because there are two steps, move each step to a separate function.

@susanodd
Copy link
Collaborator Author

susanodd commented Nov 28, 2024

The improvements are much appreciated!

That the code seems in the wrong place has to do with it being evolved several times. I wanted to show the non-backup videos as well in the print statements to look at them as well. The glosses need to be retrieved from the selected GlossVideo objects before anything is deleted.

I will split it into several commands, functions. I'm trying to ascertain how to make it the most useful to clean up the objects.
The admin facilities can either do commands on selected objects or queries / filters. I can try making a filter on the type of video file.

The API does not check the video type. That is done in the browser when you select a file. I wanted to check the webm video type but I can't upload it on Safari. But for some reason, there are webm videos from the API. I had to copy locally a webm file onto an already existing backup file in order to check that it can change the name correctly. (webm can't play on MacOS nor on Ubuntu.)

Probably we need some conversion commands, too, in the admin. It is much easier to navigate to files in the admin than on Ubuntu.

@vanlummelhuizen
Copy link
Collaborator

The glosses need to be retrieved from the selected GlossVideo objects before anything is deleted.

You are right.

@susanodd susanodd changed the title #1397: Command to delete excess backup videos and reorder versions #1397: Commands to delete excess backup videos and reorder versions Nov 29, 2024
@susanodd
Copy link
Collaborator Author

susanodd commented Nov 29, 2024

@vanlummelhuizen I added a column to the GlossVideo table in the admin. It shows the "file type" (that returned by "file".)

Lots of the "mp4" files are actually Apple iTunes Video (.M4V) Video. So they should be ".mp4" or "m4v"?

The conversion "ensure_mp4" was checking for h264 for a very long time and just making them be mp4.

Can this ever be a problem? Or should we adhere to the extension belonging to the type of video?
(I did not do this, it's very old files that are on my local computer. They are all mp4 but most are actually m4v, from Apple.)
Firefox does not care and just plays them all. The only videos it can't play are webm.

@susanodd
Copy link
Collaborator Author

Regarding the requests for code improvement

  • There are ways of reducing the if DEBUG_VIDEOS: clauses to one line if there is only a print statement. E.g something like print('bla') if DEBUG_VIDEOS else None.

I prefer not to do this, since it makes it difficult to skip over lines because of lack of indentation.

The other changes I have made.

change file extension command, match file type
@susanodd susanodd changed the title #1397: Commands to delete excess backup videos and reorder versions Commands to delete excess backup videos and reorder versions Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants