search

magenta / note-seq

A serializable note sequence representation and utilities.
Apache License 2.0
210 stars 57 forks source link

Importing note-seq changes PrettyMIDI behavior for corrupt MIDI files. #29

Open christhetree opened 3 years ago

christhetree commented 3 years ago

I recently came across this bug while processing LMD with my own scripts and note-seq chord inference. Usually, when processing a corrupt MIDI file, pretty midi throws an exception. However, if note-seq is imported, no exception is thrown anymore.

Reproducible code:

from pretty_midi import PrettyMIDI
# import note_seq  # Uncomment this line to prevent exception from being thrown

pm = PrettyMIDI('d6174dfc21a1449dc423c58065f14173.mid')  # Exception should be thrown about too many ticks

This was on python 3.8 with a clean virtualenv environment and the latest version of pretty midi and note-seq. Naturally this kind of behavior causes a lot of problems downstream if you are relying on exceptions to be thrown for skipping corrupt MIDI files.

Let me know if it's reproducible, maybe I'm missing something on my side. GitHub doesn't let me attach MIDI files, but you can find the corrupt file in the Lakh MIDI dataset in the d folder.

christhetree commented 3 years ago

Possibly related to https://github.com/magenta/note-seq/issues/27 If an exception is not thrown for corrupt MIDI files, an OOM error kills the process eventually.

cghawthorne commented 3 years ago

I'm guessing it's because of this line: https://github.com/magenta/note-seq/blob/master/note_seq/midi_io.py#L32

If you reset that constant to its default, do you get the desired behavior? This is a little tricky because we need that line to read some of our datasets, so I'm not sure what the best general solution is.

christhetree commented 3 years ago

Thanks, that's the problem.

For now I've found a workaround on my end, but yeah I see the issue with this. Forking PrettyMIDI or setting and unsetting the variable each time are probably not the best ways to handle this. On the other hand, I would imagine users importing note-seq alongside their own code that uses PrettyMIDI will be a common scenario, so changing the package for everyone can result in situations like mine.

Maybe a warning about this behavior would suffice?

cghawthorne commented 3 years ago

That seems reasonable. Where would be a good place to put the warning?

christhetree commented 3 years ago

I was thinking in the README under the installation section. I can submit a PR in a bit for it.

cghawthorne commented 3 years ago

That would be great, thanks!