Skip to content

experiment: refactor prepro datatypes - #7302

Draft
maverbiest wants to merge 6 commits into
mainfrom
refactor-prepro-datatypes
Draft

experiment: refactor prepro datatypes#7302
maverbiest wants to merge 6 commits into
mainfrom
refactor-prepro-datatypes

Conversation

@maverbiest

@maverbiest maverbiest commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Not fully polished

I was preparing to work on #7247 and thought the way we get the group_id from UnprocessedData and UnprocessedAfterNextclade in get_output_metadata currently is quite messy.

I somehow completely blanked that I'd reviewed this PR #6980 and ended up with a similar concept (probably unconsciously plagiarising that PR!), with some differences:

I'd argue we should merge #6980 rather than this PR: it's a smaller diff with less churn. But I wanted to post this as a draft since there are some different ideas on the data model that we could discuss or come back to in the future.

IMO UnprocessedEntry and UnprocessedAfterNextclade are quite natural places for the Context to move in the long run: it's context that remains the same from the time the entry is parsed until it is submitted back to the backend, and things like the accessionVersion and group_id are accessed in different places along the way (currently via dict["key"]). It would also get rid of the inconsistency where for UnprocessedAfterNextclade we need to do this dance to get the group_id and submitted_at:

group_id = (
int(unprocessed.inputMetadata["group_id"])
if unprocessed.inputMetadata["group_id"]
else None
)
submitted_at = unprocessed.inputMetadata["submittedAt"]

but not for UnprocessedData:

group_id = unprocessed.group_id
submitted_at = unprocessed.submittedAt

Screenshot

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: Add preview label to enable

@claude claude Bot added preprocessing Issues related to the preprocessing component deployment Code changes targetting the deployment infrastructure labels Sep 10, 2026
@maverbiest

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @maverbiest's task in 2m 51s —— View job


Code Review

  • Gather context (PR diff, changed files)
  • Read changed files in preprocessing/nextclade
  • Review datatype refactor for correctness, readability, maintainability
  • Post review feedback

Overall this is a clean, well-motivated refactor. Introducing SubmissionContext to carry submission-level info (accession/version, submitter, group_id, submittedAt, submissionId, is_insdc_ingest_group) consolidates data that was previously threaded through many function signatures individually (accession_version, group_id, submitted_at, args["is_insdc_ingest_group"], args["ACCESSION_VERSION"]). Nice touches:

  • Using InitVar[int] for insdc_ingest_group_id + field(init=False) for the derived is_insdc_ingest_group in SubmissionContext (datatypes.py:95-107) is an elegant way to precompute the flag once instead of recomputing it via args["is_insdc_ingest_group"] in many processing functions.
  • values.yaml:1884 correctly drops the now-unneeded submissionId: submissionId input mapping for build_display_name, matching the updated docstring in processing_functions.py:1096-1109 (submissionId now comes from submission_context instead of input_data). I checked and this is the only place in the repo that configures build_display_name, so no other configs were missed.
  • prepro.py:717-723: upload_flatfiles now skips building/uploading an EMBL flatfile when the entry already has processing errors, with a clear comment explaining why. This is a genuine behavior improvement over the previous code, which (for the unaligned/no-nextclade-alignment path) always had group_id=None on SubmissionData and would raise + append an extra "Failed to create or upload EMBL file" error on top of pre-existing errors for every entry in that path. Good catch/fix, worth calling out explicitly in the PR description since it's a behavioral change beyond a pure refactor.
  • SubmissionData.submitter/group_id moving from Optional (defaulting to None) to required fields (datatypes.py:184-186) is consistent with all construction sites now always having a SubmissionContext to pull from.

Minor nits (non-blocking):

  • processing_functions.py:1146-1157: insdc_ingested = bool(submission_context.is_insdc_ingest_group) is computed and then passed into parse_identifier_string(...) alongside submission_context itself, which already exposes .is_insdc_ingest_group. Since parse_identifier_string also receives submission_context (for the extract_regex call), the separate insdc_ingested parameter is redundant and could be dropped in favor of reading submission_context.is_insdc_ingest_group inside the function — slightly simpler signature.
  • datatypes.py:106: SubmissionContext.__post_init__ doesn't validate group_id/insdc_ingest_group_id types beyond the type hints; since backend.py:117 now explicitly does group_id=int(json_object["groupId"]), this is fine in practice, just flagging that the int cast moved from implicit to explicit (a correctness improvement, not an issue).
  • Test coverage looks solid — make_submission_context in tests/factory_methods.py:32-51 nicely prevents constructing a contradictory (group_id, is_insdc_ingest_group) pair, and all affected tests (test_host_name_validation.py, test_assign_custom_lineage.py, test_metadata_processing_functions.py, test_nextclade_preprocessing.py) were updated consistently with the new SubmissionContext-based construction.

I did not find leftover references to the removed UnprocessedData type, stale args["is_insdc_ingest_group"]/args["ACCESSION_VERSION"] accesses, or other inconsistencies from the rename.

Not verified: I don't have ruff/pytest/mypy available in this sandboxed environment (no network access to install them), so I was unable to actually execute lint/type-check/tests to confirm the suite passes — please make sure CI is green before merging. Given the PR is marked "experimental" and no PR checklist items are checked, you may still be iterating — happy to take another pass once tests are added/CI runs.
(branch)

@maverbiest maverbiest changed the title experimental: refactor prepro datatypes experiment: refactor prepro datatypes Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment Code changes targetting the deployment infrastructure preprocessing Issues related to the preprocessing component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant