Skip to content

Add Elembio, and generic, runfolder and lims shim classes#965

Draft
dkj wants to merge 5 commits intowtsi-npg:develfrom
dkj:elembio-maybe
Draft

Add Elembio, and generic, runfolder and lims shim classes#965
dkj wants to merge 5 commits intowtsi-npg:develfrom
dkj:elembio-maybe

Conversation

@dkj
Copy link
Copy Markdown
Member

@dkj dkj commented Mar 15, 2026

Add the required runfolder and lims querying methods (in new classes) to support npg_seq_pipeline running analyses, see wtsi-npg/npg_seq_pipeline#918

dkj added 5 commits March 15, 2026 16:12
- Introduced npg_tracking::elembio::runfolder for Elembio runfolder metadata handling.
- Created npg_tracking::runfolder as a generic wrapper for runfolder functionality.
- Implemented manufacturer methods for both Elembio and Illumina runfolders.
- Added tests for the new runfolder classes and their functionalities.
providing LIMS info from flowcell identification without product metrics
Cleanup the earlier Illumina based with elmbio overrides code:
- Introduced a new role, npg_tracking::runfolder::folder, to encapsulate shared runfolder path and staging behavior.
- Delegated metadata parsing to specific implementations for Elembio and Illumina runfolders.
- Updated runfolder_path_is_valid method to recognize both Elembio and Illumina runfolder structures.
- Enhanced test suite to validate the public API consistency between generic and concrete runfolder implementations.
- Improved error handling and path inference logic for runfolder discovery.
with delegated attributes and methods for improved functionality especially for more backward compatibility
@dkj dkj marked this pull request as ready for review March 26, 2026 11:58
@mgcam
Copy link
Copy Markdown
Member

mgcam commented Apr 8, 2026

Extending st::api::lims is a separate concern and should be in a separate pr. However, I question why a generic one is needed. In the context of the pipeline we always know the manufacturer and can use an appropriate driver. If a different starting point (a lims reference) is needed, either the existing drivers can be extended or new ones can be build on the top of existing.

Copy link
Copy Markdown
Member Author

@dkj dkj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm - pretty sure I have the wrong column being used for Ultima - I need to fix or just remove the Ultima example...


return [
$self->mlwh_schema->resultset('UseqWafer')->search(
{batch_for_opentrons => $self->id_flowcell_lims},
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{batch_for_opentrons => $self->id_flowcell_lims},
{id_wafer_lims => $self->id_flowcell_lims},

q[Ultima Genomics] => {
driver_class => q[st::api::lims::ml_warehouse_flowcell::ultima],
resultset => q[UseqWafer],
query_column => q[batch_for_opentrons],
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
query_column => q[batch_for_opentrons],
query_column => q[id_wafer_lims],

@dkj
Copy link
Copy Markdown
Member Author

dkj commented Apr 8, 2026

Extending st::api::lims is a separate concern and should be in a separate pr.

lims and runfolder are together as the combination of changes I needed for wtsi-npg/npg_seq_pipeline#918 - they can be separated. (But meanwhile I'll fix the title and description)

However, I question why a generic one is needed. In the context of the pipeline we always know the manufacturer and can use an appropriate driver. If a different starting point (a lims reference) is needed, either the existing drivers can be extended or new ones can be build on the top of existing.

I think you're right, I think we should know the manufacturer and so be able to identify a lims driver to use, however:

  • the id_flowcell_lims / id_wafer_lims should be unique for our Illumina/Elembio/Ultima runs so we should not need to use this extra query datum when asking for LIMS info (my original motivation I recall)
  • I'd need to have some mapping of manufacturer to lims driver in npg_seq_pipeline (not sure I prefer that)

The main motivation for the these lims shim driver additions was to be able to work purely from LIMS info, i.e. before NPG has populated any *_product_metrics or other tables in MLWH (as pipeline may need to run before that happens).

[edit: also a second look after a break makes me think there is quite a lot of refactoring possible in this PR. Flipping back to draft and 🙏 @mgcam ]

@dkj dkj changed the title Add Elembio and generic runfolder classes with tests Add Elembio and generic runfolder and lims shim classes Apr 8, 2026
@dkj dkj changed the title Add Elembio and generic runfolder and lims shim classes Add Elembio, and generic, runfolder and lims shim classes Apr 8, 2026
@dkj dkj marked this pull request as draft April 8, 2026 16:32
@mgcam
Copy link
Copy Markdown
Member

mgcam commented Apr 8, 2026

The main motivation for the these lims shim driver additions was to be able to work purely from LIMS info, i.e. before NPG has populated any *_product_metrics or other tables in MLWH (as pipeline may need to run before that

Yes, that would be very useful. However, I am not sure this code achieves this goal since it still links into product metrics tables to get tag indexes. Probably there is a default to use sequential numbers, which I have not noticed. However, (1) sequential numbers will be inconsistent with the tag indexes we are generating now and (2) since for eseq and useq controls are not represented in the LIMS part, we will have extra fun generating deplexing metrics. I should look at the Elembio pipeline results to see how this is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants