Conversation
|
This PR may be related to: #7120 ("If file sharing is enabled, bulk submission of compressed metadata with files is broken"), which matches the compressed metadata file mapping issue described in this PR's title. |
|
Claude finished @tombch's task in 2m 50s —— View job Claude Code Review
The refactor is a good direction. Threading I left 5 inline comments. The one worth acting on before merge:
The rest: a swallowed error in Two smaller notes not worth inlining:
Not verified: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const submissionFileMapping = await parseSubmissionFileMapping( | ||
| mFile, | ||
| submissionDataTypes.files?.categories?.map((category) => category.name) ?? [], | ||
| ); | ||
| if (submissionFileMapping.isErr()) { | ||
| return { type: 'error', errorMessage: submissionFileMapping.error.message }; | ||
| } | ||
|
|
||
| // Validate the submission file mapping | ||
| const validation = validateSubmissionFileMapping( | ||
| submissionFileMapping.value, | ||
| fileSharingConfig, | ||
| ); | ||
| if (validation.isErr()) { | ||
| return { type: 'error', errorMessage: validation.error.message }; | ||
| } | ||
|
|
||
| // Resolve the submission file mapping against file mapping of uploads | ||
| const { submissionFileMapping: resolvedFileMapping, fileLinkage } = resolveFileMappings( | ||
| submissionFileMapping.value, | ||
| fileMapping, | ||
| ); | ||
| const linkageErrorMessage = getLinkageErrors(fileLinkage); | ||
| if (linkageErrorMessage !== undefined) { | ||
| return { type: 'error', errorMessage: linkageErrorMessage }; | ||
| } | ||
|
|
||
| // Apply resolved mapping to metadata file | ||
| const metadataWithFileMapping = await applyFileMappings(mFile, resolvedFileMapping); | ||
| if (metadataWithFileMapping.isErr()) { | ||
| return { type: 'error', errorMessage: metadataWithFileMapping.error.message }; | ||
| } | ||
| mFile = metadataWithFileMapping.value; |
There was a problem hiding this comment.
When columnMapping === null (the common case), mFile is still the original ProcessedFile, so parseSubmissionFileMapping (line 200) and applyFileMappings (line 228) each call .text() on it independently. For a CompressedFile that means decompressing the whole metadata file twice on submit — and fflate.decompressSync/fzstd.decompress are synchronous, so both block the main thread. The useEffect above has usually already decompressed it a third time.
Converting once up front would avoid this, e.g. mFile = new VirtualFile(await mFile.text(), 'metadata.tsv') before the extraFilesEnabled block (wrapped in try/catch, or via a small helper that returns a Result). Not a correctness issue, but it's a user-visible freeze on large compressed uploads — exactly the files #7120 is about.
| setSubmissionFileMapping={vi.fn()} | ||
| onError={vi.fn()} | ||
| fileSharingConfig={{ disableStrictFilenameValidation: false }} | ||
| fileMapping={undefined} |
There was a problem hiding this comment.
This spec only gets the two new props added to keep it compiling — none of the new behaviour is covered. The three things this PR claims to fix all live in the file factory and are untested:
- A compressed (
.tsv.zst/.gz) metadata file with afiles.*column produces a correctly rewritten metadata file (If file sharing is enabled, bulk submission of compressed metadata with files is broken #7120) — currently nothing exercisesparseSubmissionFileMapping/applyFileMappingsagainst aCompressedFile. - Submitting before the
useEffecthas settled still yields the right mapping (fix(website): wait for the metadata file when submitting, don't reject #7270) — the whole point of addingparseSubmissionFileMappingto the submit path. - The factory returns
{ type: 'error' }with the right message when the metadata can't be read or the linkage is broken.
At minimum a test for (1) would be valuable, since it's the regression the PR is named after and it's cheap to write with a gziped fixture through METADATA_FILE_KIND.processRawFile.
| return async (): Promise<Result<SequenceData, Error>> => { | ||
| switch (inputMode) { | ||
| case 'form': { | ||
| const submissionId = editableMetadata.getSubmissionId(); |
There was a problem hiding this comment.
anya thoughts: can we split these out into subfunctions for form and bulk (its hard to see what is in the form and bulk case as the function is getting quite long)
| if (enableConsensusSequences && sFile === undefined) { | ||
| return { type: 'error', errorMessage: 'Please specify a sequences file.' }; | ||
| if (extraFilesEnabled) { | ||
| // Parse submission file mapping from the metadata |
There was a problem hiding this comment.
mayeb explain this is to prevent a race condition
| const newFileContent = Papa.unparse([headers, ...newRows], { delimiter: '\t', newline: '\n' }); | ||
|
|
||
| return ok(new VirtualFile(newFileContent, 'remapped.tsv')); | ||
| } catch (error) { |
There was a problem hiding this comment.
would be nice to cover this error in a test
|
|
||
| return ok({ columns, rows }); | ||
| return ok({ columns, rows }); | ||
| } catch (error) { |
2 participants
resolves #7105
resolves #7120
resolves CI flake outlined in #7270
Summary
This PR refactors the file mapping functions to use
RawFileandVirtualFiletypes fromfileProcessing.ts, that both implement theProcessedFileinterface. With this we remove the issues related to compressed metadata files in issue #7120 as now theProcessedFile.text()is called rather than the rawFile.text(). We also now have error handling for the metadata file inparseMetadataFileto address #7105 , as well as adding a call toparseSubmissionFileMappingwhen a user clicks submit, to remove the CI flake mentioned in #7270.Changes per file:
fileMapping.tsparseSubmissionFileMappingandapplyFileMappingsfunctions to takeProcessedFiletypes rather than rawFileobjects.applyFileMappingsnow returns aVirtualFile(implementsProcessedFile) instead of a rawFile.parseMetadataTextfunction into aparseMetadataFilefunction, which accepts aProcessedFile, and does error handling if anything goes wrong in reading the file.ColumnMapping.tsapplyTofunction to return aneverthrowResultcontaining aVirtualFile(implementsProcessedFile) instead of a rawFileobject, and addedneverthrowerror handling.applyTofor empty metadata files, and return aPlease provide a non-empty Metadata filerather thanTypeError: Cannot read properties of undefined (reading 'findIndex')EditPage.tsxRawFile(implementsProcessedFile) input type forapplyFileMappings.DataUploadForm.tsxDataUploadFormto theFormOrUploadWrapper.validateFileUploadStatesto check extra files upload states before file factory call.FormOrUploadWrapper.tsxFormOrUploadWrapperfile factory now has all metadata/sequence file operations (column mapping, file mapping) done in here rather than in two stages, and returns the final metadata and sequence file objects. With the column mapping updated to return aProcessedFileit also means these can be simply applied one after the other.parseSubmissionFileMappingstep within the submit handling, to fix CI flakes where submit was clicked before the file mapping had finished being parsed in the metadata.Resultinstead of custom ok/error types, and renamed variables related to file mapping.Screenshot
For #7105: Better error message with corrupted metadata file (raises on adding metadata file, and blocks submit):
For #7120:
File mappings for compressed metadata are displayed:
Uploads in progress block:
Files are mapped correctly after submit:
PR Checklist
🚀 Preview: https://file-mapping-metadata-par.loculus.org