Arvados

Updates at 93c92875aaebe5b06f8dbfe2822b59a772895c08 (branch 10111-cr-provenance-graph)

Radhika: While I start working on the tests, I would like to check with you if this is the correct approach, and if there are missing elements on the graph to be included.

Merged master && added some tests: 4ccbea9ef
Test run: https://ci.curoverse.com/job/developer-run-tests/247/

For a CR -> Use Inputs from Mounts + output uuid + log uuid as the nodes

And then need to get all child CRs for this CR and repeat the above.

Updates at 30146198f
Test run: https://ci.curoverse.com/job/developer-run-tests/251/

• Removed container_image and requesting_container from the graph.
• Added child CRs with their own mounts/output/log.

The graph is looking pretty good. A few observations:

• As we discussed, I think we should only use input collections (plus output and log uuids) for the CR in “find_collections cr[:mounts]” and exclude any other collections, if any. I think you probably need to look for collection uuids / pdhs in the segment returned by application_helper.get_cwl_inputs?

• extra white space at line ends in _show_provenance.html.erb

Thanks.

Updates: 260e85a9d

• As we discussed, I think we should only use input collections (plus output and log uuids) for the CR in “find_collections cr[:mounts]” and exclude any other collections, if any. I think you probably need to look for collection uuids / pdhs in the segment returned by application_helper.get_cwl_inputs?

Ah yes, I thought that all mounts that specified a UUID/PDH were implicitly an input.

I have changed the code so it searches only for collections inside the /var/lib/cwl/cwl.input.json mount, that as I understand by reading the get_cwl_inputs() helper, it's the object describing the input mounts.
The result is that if a CR step is created and have mounts that aren’t from a CWL definition (example: bwa command execution that uses FUSE), those mounts on the child CR won’t be included in the graph (ie: 9tee4’s /container_requests/9tee4-xvhdp-29wnyz1npk9bycs#Provenance), is that ok or should I search for “any collection” inside mounts when not using arvados-cwl-runner on the command?

• extra white space at line ends in _show_provenance.html.erb

Oops! done.

Another question: Currently I’m showing the Provenance tab on those CRs with state != Uncommitted, should I change that to only CR in Final state?

I have changed the code so it searches only for collections inside the /var/lib/cwl/cwl.input.json mount, that as I understand by reading the get_cwl_inputs() helper, it's the object describing the input mounts. The result is that if a CR step is created and have mounts that aren’t from a CWL definition (example: bwa command execution that uses FUSE), those mounts on the child CR won’t be included in the graph (ie: 9tee4’s /container_requests/9tee4-xvhdp-29wnyz1npk9bycs#Provenance), is that ok or should I search for “any collection” inside mounts when not using arvados-cwl-runner on the command?

Yes, this seems problematic. I think we should check for /keep/<pdh> format instead in mounts to get the input collections. Please confirm with Peter. Thanks.

Another question: Currently I’m showing the Provenance tab on those CRs with state != Uncommitted, should I change that to only CR in Final state?

Comparing with pipeline_instances and jobs, this seems correct (to show graph for Queued etc)

Update at: 88c241d7c
Test run will be on: https://ci.curoverse.com/job/developer-run-tests/257/

Search for all PDHs on "mounts" on cases when cwl.input.json is not included. As talked with Radhika & Bryan, outputs aren't listed using PDHs, just paths. So there's no possibility of including an output as an input.

Update at: edfc619e6

As requested on the sprint review meeting, changed the graph edges from "cr" to "child" and "mounts" to "input".

LGTM @ edfc619

In the interests of time, let's limit it to these changes:

• Needs to use PDH so that inputs and output match up. For example, in this graph the output of "rev" is supposed to be an input of "sort": https://workbench.9tee4.arvadosapi.com/container_requests/9tee4-xvhdp-w382mn52hn18oad#Provenance (this is the most important change, because the current behavior it is effectively a regression from the equivalent functionality for jobs)

• Don't render "log" outputs. They are just clutter.

• To determine the inputs of a container request, recursively search "mounts" for JSON fields that look like "portable_data_hash": "abc+123" and "location": "keep:abc+123" (this should ensure that nothing is missed)

Fixes at a4a8d41f6 - branch 10111-cr-prov-regression-fixes
Test run: https://ci.curoverse.com/job/developer-run-tests/273/

Branch 10111-collection-labels - commit 39755f764
Test run: https://ci.curoverse.com/job/developer-run-tests/274/

Added better collection labelling on CR provenance graph.

An integration test was failing, updated fix at e01823785
New test run: https://ci.curoverse.com/job/developer-run-tests/275/

Additional comment: where we have a container request with an explicit output_uuid, make sure to use the label corresponding to the name of the collection in output_uuid, before falling back on the logic outlined in note-24

Updates at 4259263d2
Test run: https://ci.curoverse.com/job/developer-run-tests/276/

Addressed issue about naming output collections after the cr's output_uuid collection reference.

How hard would it be to fix the hyperlinks so that when you have a specific UUID associated with a collection, clicking on it takes you directly to it and not to the "this PDH has multiple collections" page?

Another note. For labeling, if there are multiple collections but they have the same name, you don't need the "+N more"

It's making a separate API call for every collection. That adds a lot of latency. It should find all the PDHs in the graph, make a batch request for them all, and then filter on the workbench side.

Updates at b29ca38e4
Test run: https://ci.curoverse.com/job/developer-run-tests/278/

• Refactored the graph creation code for CR so that it minimizes the amount of API calls when looking for information about outputs, inputs and childs.
• For input collections, when there are more than one with the same name, don't add the "+N more" to the name label.
• For output collections, added an option on describe_node() helper function so that the graph node is referenced by PDH, but link urls are rendered by UUID so they take the user to the specific collection page when clicking on it.

Ok, for large workflows, it still takes forever to load, but it seems that "dot" is the bottleneck now. We need to rethink representation, but not for this story (I'm putting that on a new ticket, #11680).

On the implementation:

The intended way to call GenerateGraph() was with pdata to contain all the nodes that will be used in the graph. In order to have better separation of concerns, would it make sense for the new code in generate_provenance_edges() that does the batch queries to move to container_requests_controller#generate_provenance ?

Updates at 795bf007c
Test run: https://ci.curoverse.com/job/developer-run-tests/284/

Moved the code related to API requests to the CR controller.

Thanks. This is a much better separation of concerns.

I'm unhappy with how it behaves with large graphs, but instead of continuing to go around back and forth I think we should merge 795bf007cbe24775bd348fb40fc5c28d93c8f23d and schedule a grooming session to figure out how rendering can be improved.

LGTM.