Arvados

21074-workflow-collection-link @ a42dbb3387baf22ed7735461680fa030111e4a15

developer-run-tests: #4725

21074-workflow-collection-link @ c7c8afeb098fa6cd94ab29bee7947076946c1eff

developer-run-tests: #4726

21074-workflow-collection-link @ 2b56a3e1c4d6fe3db5957c3e1bb68833ba36c224

developer-run-tests: #4727

• All agreed upon points are implemented / addressed. Describe changes from pre-implementation design.
  • Adds collection_uuid to workflow record
  • Adds synchronization between collection records with type: workflow and workflow records linked by collection_uuid
  • Adds ability to query linked collection properties through workflows, e.g. [collection.properties.category,=,WGS]
  • Adds ability to fetch collection records linked to workflow records using include=collection_uuid when using group.contents API
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • Arvados-cwl-runner client support will be added in ticket #22761
• Code is tested and passing, both automated and manual, what manual testing was done is described.
  • Added a bunch of tests for all the new behaviors
• New or changed UX/UX and has gotten feedback from stakeholders.
  • n/a
• Documentation has been updated.
  • updated workflows API page about linked workflows
  • updated group contents API page to mention that collection_uuid can be used with include
• Behaves appropriately at the intended scale (describe intended scale).
  • Workflows are updated infrequently. Scale concerns around accessing workflow and collection records don't change. Collection records representing workflows get slightly heavier with the introduction of cwl_inputs and cwl_outputs properties but these are still significantly smaller than e.g. most mounts sections.
• Considered backwards and forwards compatibility issues between client and server.
  • Yes, this entire design is a compromise intended to maintain backwards compatibility. Older versions of arvados-cwl-runner will be able to create and consume workflows as if nothing changed. Workbench can support both legacy workflows and workflows that are linked to collections without forcing a migration.
• Follows our coding standards and GUI style guidelines.
  • yes.

Feedback on 21074-workflow-collection-link @ 2b56a3e1c4

The first line of Collection#update_linked_workflows seems to create a loophole. If a workflow and collection are linked, then a client deletes {type:workflow} from the collection properties, then the rest of the checks get bypassed -- the collection update is allowed but the linked workflow doesn't get synced or unlinked. I get the idea of avoiding the extra workflows table lookup when updating a non-linked collection though -- maybe the right condition is new_record? || properties_was['type'] != 'workflow' ?

FWIW I think the rails association lets us say just workflows as shorthand for Workflow.where(collection_uuid: self.uuid).

Collection.cwl_shortname has some commented-out code.

Workflow.validate_collection_uuid checks for arv:workflowMain in collection properties, but this seems to be redundant -- Collection#update_linked_workflows checks that too, but with a different error message. (Ditto for type -- maybe better to check all of the required properties in one place?)

The "collection is missing cwl_inputs" "collection cwl_inputs wrong type" tests are a little repetitive and fragile. Maybe this could be a parameterized test where we start with a valid collection, remove property X, confirm we can't link a workflow to it, restore property X, then confirm we can link a workflow, i.e., confirm the missing property X is really what caused the 422.

I see this branch inadvertently adds #22581's schema_migrations row, then removes it. If this branch merges after #22581, make sure git doesn't do the wrong thing (add=noop, then remove=remove).

Tom Clegg wrote in #note-60:

Feedback on 21074-workflow-collection-link @ 2b56a3e1c4

The first line of Collection#update_linked_workflows seems to create a loophole. If a workflow and collection are linked, then a client deletes {type:workflow} from the collection properties, then the rest of the checks get bypassed -- the collection update is allowed but the linked workflow doesn't get synced or unlinked. I get the idea of avoiding the extra workflows table lookup when updating a non-linked collection though -- maybe the right condition is new_record? || properties_was['type'] != 'workflow' ?

I approached this a different way, which is to add a validation that prevents you from changing "type: workflow" as long as there are linked workflows.

FWIW I think the rails association lets us say just workflows as shorthand for Workflow.where(collection_uuid: self.uuid).

I chose to leave it the way it is.

Collection.cwl_shortname has some commented-out code.

The commented-out code more accurately reflected the behavior of the original Python code, which I left for reference, however the behavior of the Ruby URI parser is very different, so the simplistic solution ended up being better -- I deleted the commented-out code.

Workflow.validate_collection_uuid checks for arv:workflowMain in collection properties, but this seems to be redundant -- Collection#update_linked_workflows checks that too, but with a different error message. (Ditto for type -- maybe better to check all of the required properties in one place?)

I removed the checks from validate_collection_uuid and tweaked update_linked_workflows so it returns a string for conditions where it didn't update anything (which can be interpreted as an error or not based on the context).

The "collection is missing cwl_inputs" "collection cwl_inputs wrong type" tests are a little repetitive and fragile. Maybe this could be a parameterized test where we start with a valid collection, remove property X, confirm we can't link a workflow to it, restore property X, then confirm we can link a workflow, i.e., confirm the missing property X is really what caused the 422.

I'm looking at these two tests and I have to be honest with you, I cannot think of a way to refactor them that ends up being shorter and simpler than the two tests on their own, although it sounds like you are also asking for the tests check the validation more extensively, in which case that might make sense.

I see this branch inadvertently adds #22581's schema_migrations row, then removes it. If this branch merges after #22581, make sure git doesn't do the wrong thing (add=noop, then remove=remove).

I squashed and rebased the whole thing to fix that.

Here's the branch back, but I guess I'll have to spend a little more time with the tests, but you can look at the other changes?

21074-workflow-collection-link @ 93b97de3ac50eddcca0275ad16028010be8cdb8f

developer-run-tests: #4736

oh! also whatever solution we come up with for #22785 will almost certainly also be needed here, if the solution doesn't automatically generalize.

• when a workflow is updated, we would do

  wfcoll = Collection.find_by_uuid(collection_uuid)
  wfcoll.validate_for_workflow
  wfcoll.update_linked_workflows([self], save: false)
  

• when a collection is updated, we would do

  if !new_record? && properties_was["type"] == "workflow" && workflows.any?
    validate_for_workflow
    update_linked_workflows(workflows, save: true)
  end
  

cannot think of a way to refactor them that ends up being shorter and simpler

My main concern is that "assert_response 422" is not a very convincing way to check that "cwl_inputs wrong type" or "missing cwl_inputs" is the problem being caught in the preceding chunk of code. I guess the minimal fix would be to check that the returned error message mentions arv:cwl_inputs.

21074-workflow-collection-link 1802ef7cc2a42224631809c3b2f2aead564b0e71

Tom Clegg wrote in #note-64:

The usage/signature of the new update_linked_workflows (if error only matters to caller A then return it as a string, but if it matters to both A and B then raise an exception) seems unnecessarily hard to follow. I think the basic issue is that its two callers want different things. Could we rearrange it so update_workflows really just updates workflows, and a separate function validate_for_workflow always raises an exception if the collection is not suitable for linking to a workflow? Then,

• when a workflow is updated, we would do [...]
• when a collection is updated, we would do [...]

Yes, that is a better separation of concerns. Although it didn't seem helpful to split out a separate validate_for_workflow function, as the only thing that we care about is that the collection type is "workflow" (which can be trivially checked in the Workflow class) and it is well formed (which is already checked by update_linked_workflows.

My main concern is that "assert_response 422" is not a very convincing way to check that "cwl_inputs wrong type" or "missing cwl_inputs" is the problem being caught in the preceding chunk of code. I guess the minimal fix would be to check that the returned error message mentions arv:cwl_inputs.

Got it. I added assertions on the error messages. I also added a couple more tests on other validations reflecting the refactoring work.

Now 21074-workflow-collection-link @ 1802ef7cc2a42224631809c3b2f2aead564b0e71

developer-run-tests: #4740

Peter Amstutz wrote in #note-65:

Yes, that is a better separation of concerns. Although it didn't seem helpful to split out a separate validate_for_workflow function, as the only thing that we care about is that the collection type is "workflow" (which can be trivially checked in the Workflow class) and it is well formed (which is already checked by update_linked_workflows.

1. Any collection with properties["type"]=="workflow" must also have the other properties like arv:workflowMain (in this case we should validate arv:workflowMain etc. when updating a collection, and don't need to re-validate them when linking a workflow, as we are doing now)
2. Any collection that is linked to a workflow must have properties["type"]=="workflow", arv:workflowMain, etc. (in this case we should validate arv:workflowMain etc. when linking a collection, and it should not be an error to have properties["type"]=="workflow" without the other properties, as it is now)

In the previous code I thought you were aiming for option 2 but now I'm not sure.

Ok, from our discussion at standup, the strict restrictions on collection properties should only apply to collections that are actually linked. This accomodates legacy collections which have type: workflow but don't have all the newly defined fields. Put the tests back the way they were (but now checks the error messages) and adds comments indicating what the tests are intended to be testing.

21074-workflow-collection-link @ b38bdbf2d2d06e11bb7585898defe248c8edce65

developer-run-tests: #4741

LGTM, thanks!

Had to merge main and resolve conflicts, running tests again to be sure:

21074-workflow-collection-link @ 277745f9375dd3c4aae287cf878154eff2e46a68

developer-run-tests: #4742