fix: Agilent OpenLab CDS - accept zipped result sets - #1252
Merged
Conversation
OpenLab CDS help docs state that users may compress their .rslt result set before dropping it in the File Watched directory, but only the "rslt" extension was accepted, so a .zip upload failed with "Unsupported file extension 'zip'" (or no vendor found during auto-discovery). - Accept the "zip" extension in addition to "rslt". - Replace the unconditional sniff() with a content check, since .zip is shared with Cytiva Unicorn and AppBio Absolute Q. - Unwrap extra layers of compression, so compressing either the .rslt folder or the .rslt file itself (which is already an archive) works. - Raise AllotropeConversionError instead of bare StopIteration/BadZipFile when an archive is not a recognizable result set. Tests synthesize each compression shape from the existing result set fixture, and assert all of them produce the same ASM. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
stephenworlow
previously approved these changes
Aug 25, 2026
stephenworlow
self-requested a review
August 25, 2026 13:49
…t-zipped-result-sets # Conflicts: # src/allotropy/parsers/agilent_openlab_cds/agilent_openlab_cds_decoder.py
…t-zipped-result-sets
stephenworlow
approved these changes
Aug 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The OpenLab CDS help docs tell users they may compress their
.rsltresult set before dropping it in the File Watched directory:That was never true in allotropy.
SUPPORTED_EXTENSIONS = "rslt"meant a.zipupload was rejected before the parser saw any bytes:The
.rsltfiles we already accept are zip archives, so the contents were never the problem — only the extension gate and, for one shape, an extra layer of nesting.Changes
zipextension in addition torslt.sniff()with a content check.return Truewas only safe while.rsltwas exclusive to this parser. Withzipadded, the old sniff would have claimed every.zipand stolen files from Cytiva Unicorn ("zip") and AppBio Absolute Q ("csv,zip"). It now looks for an ACAML member.open_result_set). Compressing the.rsltfolder yields an archive whose members are the result set; compressing the.rsltfile yields an archive whose only member is another archive. Both now work.StopIteration, and an uncompressed file raisedBadZipFile. Both now raiseAllotropeConversionErrorwith a message, since malformed archives are newly reachable input.Verification
Tested against archives produced by the real macOS Finder "Compress" path (
ditto -c -k --sequesterRsrc --keepParent, which adds the__MACOSXresource forks that hand-built zips lack):IDENTICAL = byte-equal ASM against the existing fixture (~192KB of output, data cubes included), ignoring
file name/UNC path, which legitimately differ.hatch run lintclean.tests/discover_vendor_test.pypasses, confirming no cross-vendor sniff regressions.Notes for reviewers
.rsltfixture rather than checking in new files, avoiding ~3MB of duplicate test data.MAX_ARCHIVE_NESTING3 → 1): exactly the two nested cases fail, so the assertions aren't vacuous.Unsupported file extension 'zip'error above is the signal that it did.🤖 Generated with Claude Code
Update: rebased on main (2026-08-25)
Merged latest
main, which now contains #1254 (OpenLab CDS memory + optional ACAML metadata) and #1258. Resolving the overlap inagilent_openlab_cds_decoder.pychanged two things in this PR:.sqxstays optional. This PR originally routed the sequence-file lookup through a new_get_first_matching_filenamehelper that raised when.sqxwas absent. fix: Cut OpenLab CDS peak memory 4x and handle optional ACAML metadata #1254 made.sqxoptional (falling back to the ACAML method documents), and theSiriusfixture it added has no.sqx, so main's behaviour wins. The helper became dead code and was removed — the "clearer errors" bullet above now applies only to the ACAML and non-archive cases, both still covered by tests.sniffno longer copies the whole file into memory. It now callsget_seekable_bytes_stream()instead ofget_bytes_stream(). Result sets reach ~1GB, andget_bytes_stream()buffers the entire upload — using it insniffwould have silently undone fix: Cut OpenLab CDS peak memory 4x and handle optional ACAML metadata #1254's memory fix on every discovery pass.Also fixed the
Quality Checksfailure that predated the merge:from_filereturnsMapping[str, Any], notdict[str, Any], so theexpectedfixture's annotation failed mypy.Re-verified after the merge: 1498 passed (up from 1496 — main added the two
Siriusfixture cases),hatch run lintclean,tests/discover_vendor_test.pygreen.