Skip to content

Enabling publishing of library asserts to stock_resource table.#5430

Open
dasunpubudumal wants to merge 32 commits intodevelopfrom
5382-y25-697---as-a-developer-ben-i-would-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table
Open

Enabling publishing of library asserts to stock_resource table.#5430
dasunpubudumal wants to merge 32 commits intodevelopfrom
5382-y25-697---as-a-developer-ben-i-would-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table

Conversation

@dasunpubudumal
Copy link
Contributor

@dasunpubudumal dasunpubudumal commented Dec 23, 2025

Closes #

Changes proposed in this pull request

  • Change stock? method to return true for LibraryAsset.
  • Update messenger.rb to set the labware_type in the MLWH message.

Instructions for Reviewers

[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to main]
    - Check story numbers included
    - Check for debug code
    - Check version

@dasunpubudumal dasunpubudumal marked this pull request as ready for review December 23, 2025 12:13
@codecov
Copy link

codecov bot commented Dec 23, 2025

Codecov Report

❌ Patch coverage is 81.25000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.38%. Comparing base (181fa9f) to head (60182a0).

Files with missing lines Patch % Lines
app/models/messenger.rb 66.66% 5 Missing ⚠️
...anifest_excel/sample_manifest_excel/upload/base.rb 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #5430      +/-   ##
===========================================
- Coverage    87.40%   87.38%   -0.02%     
===========================================
  Files         1457     1457              
  Lines        32705    32734      +29     
  Branches      3423     3427       +4     
===========================================
+ Hits         28586    28606      +20     
- Misses        4104     4113       +9     
  Partials        15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dasunpubudumal dasunpubudumal marked this pull request as draft December 23, 2025 12:29
@dasunpubudumal
Copy link
Contributor Author

dasunpubudumal commented Dec 24, 2025

Some notes on the implementation

Tip

This comment is a way for myself to keep copious notes regarding this story. So, this comment is primarily for my benefit but I hope this would be useful for others and for code reviews as well!

This is going to be a long comment so brace yourselves 🚤

  • Each manifest asset type has its own behaviour. Listed below are the behaviours for each asset:
Asset Type Behaviour Module
1dtube SampleTubeBehaviour
plate PlateBehaviour
tube_rack TubeRackBehaviour
multiplexed_library MultiplexedLibraryBehaviour
library_plate LibraryPlateBehaviour
library LibraryTubeBehaviour
nil UnspecifiedBehaviour
  • Each behaviour has got a stock? method that determines if register_stock! function is called. However, it is inherited by the parent base, therefore I have overridden that function within the library behaviour modules.

That is essentially the solution for the first acceptance criteria.


For the second part (i.e., changing the labware_type of library assets in MLWH), we need to update the subject_type of the receptacle. This is because that the IO model for the well (or any receptacle for that matter) uses the subject_type as the labware_type. See app/models/api/messages/well_stock_resource_io.rb.

  • However, subject_type is defined at the model level; for example, see app/models/well.rb.
  • Therefore, we need to either create new models altogether for library assets, or figure out a way to patch the subject_type of the asset before the message is sent with register_stock! method1.

It would be worthwhile to mention how the stock_resource table is populated. Bear in mind that I am not diving into any RabbitMQ-related details here but note that they are crucial to populating the said table. Any important details regarding the underlying messaging flow, I would refer the reader to documentation elsewhere.

Procedural flow for sending receptacles to stock_resources table

When a sample manifest is uploaded, register_stock_resources method is called from the base class (app/sample_manifest_excel/sample_manifest_excel/upload/base.rb) that handles manifest uploads. This method figures out which receptacles need to be sent to MLWH, and invokes register_stock! method on each receptacle (emphasis on "each") that eventually sends a message to MLWH containing receptacle information. The following function determines which receptacles are sent to stock_resource table.

def stock_receptacles_to_be_registered
    return [] unless sample_manifest.core_behaviour.stocks?

    @stock_receptacles_to_be_registered ||= rows.select(&:sample_created?).map(&:asset)
end
  • core_behaviour function returns the behaviour module (e.g., PlateBehaviour, LibraryTubeBehaviour, etc.) for a certain asset type (asset_type is a query parameter for the manifest upload page). Thus, each behaviour module's stocks? function could be overriden.
  • @stock_receptacles_to_be_registered is a list of receptacles that are fetched from row.asset call.

It feels like the more natural approach - well, I should probably rephrase it as the more "object oriented way" - to setting up the subject_type is to have separate classes for each library receptacle type; i.e., LibraryPlateWell, LibraryTube, MultiplexedLibaryTube. The good news here is that models for LibraryTube and MultiplexedLibraryTube already exist! So for the latter two cases, changing the subject_type is just a matter of overriding the subject_type function to return the respective values (i.e., library_tube and multiplexed_library_tube?)

However, LibraryPlateWell does not exist. So, we need to figure out a way to either update the LibraryPlateBehaviour (which overrides PlateBehaviour) and convert the wells into LibraryPlateWell (which is the easiest), or update the logic that processes the manifest to create the LibraryTubeWell as wells for the plate if the asset_type is library_plate.

First attempt of a solution: Patching the library behaviour

I tried patching the wells' subject_type attribute before the register_stock! method is called.

def update_subject_type_for_library_receptacles!(asset)
  return unless sample_manifest.core_behaviour.to_s.include?('LibraryPlateBehaviour')

  asset.subject_type = 'library_plate_well'
end

# register_stock! is called on objects returned by the following method.
def stock_receptacles_to_be_registered
  return [] unless sample_manifest.core_behaviour.stocks?

  @stock_receptacles_to_be_registered ||= rows.select(&:sample_created?)
    .map(&:asset)
    .each do |asset|
    update_subject_type_for_library_receptacles!(asset)
  end
end

This did not work. The reason is elabrated in the note below.

Note

This stock_resource table business is done through the broadcasting strategy2 via Warren; i.e., it first publishes a shorter form of the message (e.g., [Well, 2312]), and later the consumer defined at app/models/warren/subscriber/queue_broadcast_consumer.rb broadcasts the full message to MLWH.

An interesting property about the the consumer process function is as follows:

def process
   klass = json.first.constantize
   klass.find(json.last).broadcast
 rescue ActiveRecord::RecordNotFound
   # This may indicate that the record has been deleted
   debug "#{payload} not found."
end

Note that what this does is it fetches the model type using json.first.constanize (i.e., Well, Tube, etc.) and invokes a database SELECT (i.e., find()) to fetch the model again. So, the subject_type for that model will be the one set by default with the subject_type function of the model which is @subject_type ||= 'well'. Because of this, patching the register_stock! function (or patching the subject_type before it gets to the register_stock! call) would not work.

It seems that the solution lies at the model level. It would be tricky because the behaviour of the model's subject_type function would need to be altered based on the asset_type (discussed above) - which is a sample manifest construct that is unrelated to the model itself.

Second attempt of a solution: Updating the model

I am currently checking if there's a way we could access the manifest that the asset (e.g., a well) was created from. Currently, my idea of accessing the manifest through the asset is using the association table sample_manifest_asset.

SampleManifestAsset.find_by(asset_id: well.id).sample_manifest.asset_type

So, the consumer function needs to be updated to:

def process_item(klass)
  item = klass.find(json.last)
  asset_type = SampleManifestAsset.find_by(asset_id: item.id)&.sample_manifest&.asset_type

  item.subject_type = 'library_plate_well' if asset_type.present? && asset_type == 'library_plate' && klass == Well

  item
end

However, interestingly enough, the item here is an instance of the Messenger class, not the labware class itself (e.g., Well or Tube).

#<Messenger id: 1346, target_id: 18238, target_type: \"Receptacle\", root: \"stock_resource\", template: \"WellStockResourceIo\", created_at: \"2026-01-09 12:01:51.000000000 +0000\", updated_at: \"2026-01-09 12:01:51.000000000 +0000\">

Therefore, item.subject_type does not exist and setting it would not modify the resource's labware_type.

So, this method does not work. It seems like the solution lies either in Warren (which is unlikely) or unified_warehouse.

Update the Messenger model

We could try updating the as_json() function in the Messenger model.

I have modified the as_json function as follows:

def as_json(_options = {})
  message = render_class.to_hash(target)
  { root => process_receptacles(message), 'lims' => configatron.amqp.lims_id! }
end

# Processes the message for receptacle targets, setting labware type if applicable.
# @param message [Hash] The message to process.
# @return [Hash] The processed message.
def process_receptacles(message)
  return message unless receptacle_target?

  asset_type = fetch_asset_type
  set_labware_type(message, asset_type) if library_plate?(asset_type)
  message
end

private

# Checks if the target type is 'Receptacle'.
# @return [Boolean] True if target type is 'Receptacle', false otherwise.
def receptacle_target?
  target_type == 'Receptacle'
end

# Fetches the asset type for the current target.
# @return [String, nil] The asset type or nil if not found.
def fetch_asset_type
  SampleManifestAsset.find_by(asset_id: target_id)&.sample_manifest&.asset_type
end

# Determines if the asset type is a library plate.
# @param asset_type [String, nil] The asset type to check.
# @return [Boolean] True if asset type is 'library_plate', false otherwise.
def library_plate?(asset_type)
  asset_type.present? && asset_type == 'library_plate'
end

# Sets the labware type in the message if the asset is a library plate.
# @param message [Hash] The message to update.
# @param asset_type [String, nil] The asset type (unused).
# @return [void]
def set_labware_type(message, asset_type)
  message['labware_type'] = 'library_plate_well'
end

This ended up correctly working for library plates.

Footnotes

  1. Patching is not possible. Please refer to the note in the "First Attempt" section.

  2. For those who may be interested, I have previously documented this strategy in the Warren documentation.

dasunpubudumal and others added 19 commits December 24, 2025 11:14
…t subject type assignment for library plates
…a-developer-ben-i-would-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table
…-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table
…n-i-would-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table' into y25-544-and-697
…-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table
…97---as-a-developer-ben-i-would-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table
…a-developer-ben-i-would-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table
@dasunpubudumal dasunpubudumal marked this pull request as ready for review January 19, 2026 14:28
dasunpubudumal and others added 5 commits January 26, 2026 14:26
…-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table
…a-developer-ben-i-would-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table
…n-i-would-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table' into 5382-y25-697---as-a-developer-ben-i-would-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table
…-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table
Copy link
Contributor

@BenTopping BenTopping left a comment

Choose a reason for hiding this comment

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

Thanks for the issue notes and description. Couple small comments, my main concern is around the library_plate_well logic.

class Core < SampleManifest::PlateBehaviour::Base
include SampleManifest::CoreBehaviour::LibraryAssets

def stocks?
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you update this line https://github.com/sanger/sequencescape/blob/develop/app/models/sample_manifest/core_behaviour.rb#L76 instead of overwriting each instance separately?

{ root => render_class.to_hash(target), 'lims' => configatron.amqp.lims_id! }
message = render_class.to_hash(target)
Rails.logger.info("Publishing message: #{message}")
{ root => process_receptacles(message), 'lims' => configatron.amqp.lims_id! }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this messenger just used for receptacles? If its a shared class Im not sure this logic should be here.

Copy link
Contributor Author

@dasunpubudumal dasunpubudumal Feb 2, 2026

Choose a reason for hiding this comment

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

No it is not. It is a shared class. The problem is that the register_stock! method pushes messages to the broadcast queue with the Messenger model (e.g., [Messenger, 12123]). It doesn't push messages like [Well, 12123] which makes the whole thing a bit weird and difficult. It felt weird having to put this logic here but I couldn't figure out any other way.

Copy link
Contributor Author

@dasunpubudumal dasunpubudumal Feb 2, 2026

Choose a reason for hiding this comment

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

I will try to instantiate the wells with the subject_type = "libarary_plate_well" upon manifest upload.

return message unless receptacle_target?

asset_type = fetch_asset_type
message['labware_type'] = 'library_plate_well' if library_plate?(asset_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

is library_plate_well the only affected labware type here, are the other library types okay?

…a-developer-ben-i-would-like-sample-manifest-library-assets-to-appear-in-the-stock_resource-table
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