Automatically detect gzipping in `MZReader`?

[TheLostLambda]

Hello!

Just a quick one: would you be interested in unifying the MZReader::open_read_seek() and MZReader::open_gzipped_read_seek() and the similar methods?

It would be nice to avoid needing infer_from_stream() and the manual handling!

[TheLostLambda]

I'm happy to implement this if you think it's a good idea!

[mobiusklein]

This is doable. When I wrote this machinery initially, I wasn't as comfortable with Box<dyn $TRAIT>.

It does have the potential to be a performance footgun of epic proportions. flate2's GzDecoder implementations do not implement io::Seek, so I fake it by doing incremental reads, which has to decompress the data to count bytes in the uncompressed file, just as Python's gzip.GzipFile does. This means seeking forward a little bit is okay, but if you seek backwards even a single byte, you have to rewind all the way back to the start of the file and read forwards again. It also doesn't support seeking relative to the end of the file.

That has two consequences. The first that reading the offset index or building it on open involves doing a full scan of the file with decompression, which can be slow. The second is that if your program is written with the assumption that random access is relatively inexpensive, it will be much slower. You might write your program differently to handle the case where random access is expensive, but because Rust doesn't make it easy to check capabilities at runtime, (i.e. no if implements!(x, io::Seek) { ... } else { ... }). I thought it was better to be explicit.

[mobiusklein]

How is e6587ea?

[TheLostLambda]

That commit looks good overall and is the sort of thing I was thinking, but I do wonder (as we're also thinking about that double-buffering issue) if it's worth trying to think up a way to keep the number of different available constructors from getting too out of control? I've found it mildly tricky sometimes to uncover all of the different ways to build a reader. I don't have a great solution here yet, but I'm happy to play around with builders like I mentioned in that other issue to see if something a bit tidier is possible?

It is perhaps my greatest and most consistent pain-point with Rust that we don't have stable "specialization" — that, I believe, would make it possible to use more efficient code when the underlying type is Seek and a fallback when it isn't. It's shocking how often I tend to run into that issue...

As for the Gzip efficiency, do you think it would be possible / desirable to have a seekable wrapper that moves compressed bytes from the input buffer (as they are read or seeked through) to an uncompressed buffer? Obviously that takes more memory than only keeping the compressed data, but it could solve the random access issue at least? Seeking backwards wouldn't require a complete re-decompression up to that point?

[mobiusklein]

Please take a look at feature/avoid-double-buffer which adds mzdata::io::mzml::BufferedMzMLReaderType and BufferedMzMLReaderType::new_buffered. Try the new specialized constructors to see if this solves your problem reading from Vec<u8>. There are indeed too many constructors, and they are all in the public API. A builder could help simplify user code, and I'd gladly take a PR with one. This is also a documentation problem, as MZFileReader::open_path is what the majority of users will want.

As for the Gzip efficiency, do you think it would be possible / desirable to have a seekable wrapper that moves compressed bytes from the input buffer (as they are read or seeked through) to an uncompressed buffer? Obviously that takes more memory than only keeping the compressed data, but it could solve the random access issue at least? Seeking backwards wouldn't require a complete re-decompression up to that point?

This kind of approach is impractical in the general case. It'd only work for files where you can already afford to hold the entire run in memory, so you're better off parsing over it once and storing all the spectra (in a MemorySpectrumSource if you need the API) rather than storing and reparsing the raw bytes to fetch a previous spectrum.

Rather than torment mzML and gzip with a zran implementation, or using a blocked gzip like bgzf or dictzip, future efforts are aimed at moving away from mzML entirely, with enough community momentum to hopefully get vendor buy-in.

[TheLostLambda]

The feature/avoid-double-buffer branch looks good and improves performance!

I'll prototype a way to simplify all of these constructors into a builder! It'll take me a little while, but it's an engineering mission I'm happy to tackle!

And I think that's fair about the gzip stuff... Are you a parquet promoter? I know ThermoRawFileParser can generate that, and it seemed like the way people were gradually migrating... Is that something we could support in mzdata? I would certainly mean I could adopt it!

[TheLostLambda]

Smaller even than gzipped mzML (with the data arrays also compressed), and I know Rust has some really nice crates for working with it:

[TheLostLambda]

One note on the buffering though! I got some very perplexing issues when using MzMLReader, since that type requires that R is specifically a BufReader! I had to stop using MzMLReader and change to BufferedMzMLReaderType to get the code to compiles!

[TheLostLambda]

A surprisingly recently publication about parquet for MS: https://pubs.acs.org/doi/full/10.1021/acs.jproteome.5c00435

[mobiusklein]

Yes, I'm a co-author on that paper. Also I'm writing the prototype: https://github.com/mobiusklein/mzpeak_prototyping.

mzParquet, from Michael Lazear, is a metadata-light format, akin to MGF with just enough information for what is needed to service the Sage search engine. In the implementation in ThermoRawFileParser, it's not much better. It will eventually gain a few more details when v1.5/2.0 is released and I am willing to support it behind a feature flag.

[mobiusklein]

One note on the buffering though! I got some very perplexing issues when using MzMLReader, since that type requires that R is specifically a BufReader! I had to stop using MzMLReader and change to BufferedMzMLReaderType to get the code to compiles!

Yes, that was necessary because MzMLReader<R> is by definition a different type from BufferedMzMLReaderType<R>. Without runtime overhead there is no way around that that doesn't break one name. I chose to make the new name explicit rather than break the existing name.