magenta/note-seq

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

christhetree opened this issue · 6 comments

christhetree commented

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

Possibly related to #27
If an exception is not thrown for corrupt MIDI files, an OOM error kills the process eventually.

cghawthorne commented

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

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

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

christhetree commented

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

cghawthorne commented

That would be great, thanks!