HumanCellAtlas / secondary-analysis

Secondary Analysis Service of the Human Cell Atlas Data Coordination Platform
https://pipelines.data.humancellatlas.org/ui/
BSD 3-Clause "New" or "Revised" License
3 stars 2 forks source link

[SPIKE] AUD: Make no-op working in DCP #654

Closed kbergin closed 5 years ago

kbergin commented 5 years ago

We implemented the no-op features in Lira and Falcon but it caused some problems around notifications and result in duplicate notifications/workflows and failing dcp integration tests.

In order to keep it working/fix it, there are a few steps we need to take:

  1. We need to fix the DCP test suite itself so it won’t fail at duplicated workflows (Falcon will abort them at this moment). Instead, it should fail when it found duplicated result bundles.
  2. Look into metadata-api/DSS api see if we could shortcut the checkout process if we only want the metadata-files of a bundle
  3. Talk to DSS, see if they are willing to increase the notification timeout a bit.
  4. Do research about:
    1. Move the bundle “sniffing“ logic from Lira to Falcon.
    2. Implementing a Pub/Sub queue as Andrej suggested.

┆Issue is synchronized with this Jira Story

kbergin commented 5 years ago

➤ Chengchen Wang commented:

[~sehsan]

kbergin commented 5 years ago

➤ Saman Ehsan commented:

As discussed in person, if Falcon “sniffs” the bundle that triggered each workflow and then adds the input hash label before starting it in Cromwell, this could lead to a race condition. Because there is a slight delay in applying labels to workflows in Cromwell, it is possible that Falcon thinks no workflows exist with a particular label hash when really the label hasn’t been applied yet.

kbergin commented 5 years ago

➤ Saman Ehsan commented:

Spike doc is here: https://docs.google.com/document/d/1j6EfYq17fry6HHzJYu9oYXZSZHJRQAXs_e8xllqZ8c4/edit# ( https://docs.google.com/document/d/1j6EfYq17fry6HHzJYu9oYXZSZHJRQAXs_e8xllqZ8c4/edit#|smart-link )

kbergin commented 5 years ago

➤ Saman Ehsan commented:

I did a comparison between different ways to get bundle metadata and uploaded the script I used here as a reference: https://github.com/HumanCellAtlas/pipeline-tools/blob/se_test_get_metadata/pipeline_tools/tests/shared/test_metadata_api.py ( https://github.com/HumanCellAtlas/pipeline-tools/blob/se_test_get_metadata/pipeline_tools/tests/shared/test_metadata_api.py|smart-link )

kbergin commented 5 years ago

➤ Saman Ehsan commented:

PR is here: https://github.com/HumanCellAtlas/pipeline-tools/pull/162 ( https://github.com/HumanCellAtlas/pipeline-tools/pull/162|smart-link )

I’ll test this with sending notifications in our dev environment later today.

brianraymor commented 5 years ago

@jkaneria - I've added this to Ensure that simple field-level .. epic. Now that the spike is complete, is this a blocking issue or simply an implementation issue that's part of the epic which needs a Release and Milestone? Also - In Progress pipeline?

kbergin commented 5 years ago

➤ Saman Ehsan commented:

From testing the pipeline-tools PR in dev, we observed that removing the data file checkout did not reduce the pre-processing time enough to prevent the request from the data store timing out. Additionally, the time varies depending on the number of metadata files in the bundle, which could increase over time and break this implementation.

So out of the options outlined in the document, we decided to move forward with adding a google pub/sub queue.

brianraymor commented 5 years ago

Discussed on the August 15 Refinement call: @jankeria to:

  1. Update this issue to reflect appropriate modeling. Since the original Spike is complete, this issue is not blocking the Ensure that simple field-level metadata updates are propagated to downstream DCP components. epic. There's simply pending work at this point.
  2. Add any pending Secondary Analysis work items to the parent epic (222 above)
  3. Assign a Release and Milestones to those work items - confirming completion in Milestone 2.
  4. Move work items an In Progress-ish pipeline from New.