Project

General

Profile

Actions

Feature #19070

closed

--create/update-workflow should take owner into account uploading dependencies

Added by Peter Amstutz almost 2 years ago. Updated almost 2 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
CWL
Target version:
Story points:
-
Release relationship:
Auto

Subtasks 2 (0 open2 closed)

Task #19096: Review 19070-update-workflow-depsResolvedPeter Amstutz05/04/2022Actions
Task #19134: Fix integration test on jenkins ResolvedPeter Amstutz05/04/2022Actions

Related issues

Related to Arvados Epics - Idea #17848: CWL runner improvementsResolved07/01/202103/29/2023Actions
Actions #1

Updated by Peter Amstutz almost 2 years ago

  • Status changed from New to In Progress
Actions #2

Updated by Peter Amstutz almost 2 years ago

  • Assigned To set to Peter Amstutz
  • Category set to CWL
  • Tracker changed from Bug to Feature
Actions #3

Updated by Peter Amstutz almost 2 years ago

  • Subject changed from --update-workflow should take owner into account uploading dependencies to --create/update-workflow should take owner into account uploading dependencies
Actions #4

Updated by Peter Amstutz almost 2 years ago

  • Related to Idea #17848: CWL runner improvements added
Actions #5

Updated by Peter Amstutz almost 2 years ago

19070-update-workflow-deps @ bea50608be40f5ecf218fa5be75213579ad95eca

  • add --copy-deps option, when enabled, dependencies of the workflow (input collections and docker images) are copied into the destination project
  • copy-deps is default behavior for --create-workflow and --update-workflow
  • when executing workflows from command line, does not copy anything by default (can still provide --copy-deps)
  • added integration test which tries the various options and checks the results

developer-run-tests: #3086

Actions #6

Updated by Peter Amstutz almost 2 years ago

19070-update-workflow-deps @ 1eb881796a672247db251fd7298b0a7253f83259

  • Everything from above, but also make --match-submitter-images default behavior for --create-workflow and --update-workflow

developer-run-tests: #3096

Actions #8

Updated by Lucas Di Pentima almost 2 years ago

Some questions & comments:

  • In file sdk/python/arvados/commands/keepdocker.py:L410 Is there an advantage of uploading the docker image as a first attempt instead of using one that's already in Arvados?
  • Typo 'depenencies' at --no-copy-deps's help message
  • In file sdk/cwl/arvados_cwl/executor.py: L547 refers to runtimeContext.update_workflow but L554 refers to self.runtimeContext.update_worflow, how both context differentiate between each other? the one passed to arv_executor() should have porecedence?
  • This behavior change may need some documentation updates and probably a migration note, in case clients rely on the old way of doing things, wdyt?
Actions #9

Updated by Peter Amstutz almost 2 years ago

  • Target version changed from 2022-05-11 sprint to 2022-05-25 sprint
Actions #10

Updated by Peter Amstutz almost 2 years ago

Lucas Di Pentima wrote:

Some questions & comments:

  • In file sdk/python/arvados/commands/keepdocker.py:L410 Is there an advantage of uploading the docker image as a first attempt instead of using one that's already in Arvados?

Because if the image exists in Docker locally, the assumption is that we want to match the image id of the local copy with the Arvados copy, which means we have to find out what the local version is.

As a fallback behavior, if you don't have the image or don't have Docker, you can still use arv-keepdocker to copy an image from one project to another.

  • Typo 'depenencies' at --no-copy-deps's help message

Fixed.

  • In file sdk/cwl/arvados_cwl/executor.py: L547 refers to runtimeContext.update_workflow but L554 refers to self.runtimeContext.update_worflow, how both context differentiate between each other? the one passed to arv_executor() should have porecedence?

The code was being sloppy, some places referred to the runtimeContext that was passed in, and other places referred to the runtimeContext from arvRunner (because it wasn't passing in runtimeContext). I cleaned this up, now it uses the passed-in runtimeContext almost everywhere. The only reason I didn't do it the first time is because it was extra work (but I went ahead with it since you called it out).

  • This behavior change may need some documentation updates and probably a migration note, in case clients rely on the old way of doing things, wdyt?

I updated the documentation.

19070-update-workflow-deps @ 45e2ea82d68e74774bf61f6966e3777973e61845

developer-run-tests: #3118

Actions #11

Updated by Lucas Di Pentima almost 2 years ago

This LGTM, thanks!

Actions #12

Updated by Peter Amstutz almost 2 years ago

  • Release set to 51
Actions #13

Updated by Peter Amstutz almost 2 years ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF