Bug #13681

[CWL] Cannot set submit-runner-ram when running from Workbench

Added by Peter Amstutz about 1 year ago. Updated about 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
07/05/2018
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0
Release:
Release relationship:
Auto

Description

Workbench has the RAM allocation for arvados-cwl-runner jobs hardcoded at 256MB. This is way too stingy (when you run from the command line, the default is 1GB) and users with large collection inputs may need as much as 8GB. When running from workbench, there is no way to set --submit-runner-ram. This should be settable, at minimum when using a-c-r --create-workflow.

Proposed fix:

  • Add a "workflow_runner_ram" column to the Workflows table
  • When using "a-c-r --create-workflow" or "a-c-r --update-workflow", looks for "--submit-runner-ram" and set "workflow_runner_ram" in the request
  • When workbench constructs a container request to run a-c-r, use the value of "workflow_runner_ram" for "--submit-runner-ram" on the a-c-r command line.

Subtasks

Task #13721: a-c-r understands runner ram hintResolvedPeter Amstutz

Task #13722: Workbench understands runner ram hintResolvedPeter Amstutz

Task #13723: Review 13681-wb-acr-submit-ramResolvedPeter Amstutz

Task #13724: Document runner ram hintResolvedPeter Amstutz


Related issues

Related to Arvados - Bug #13766: [API] bug in libyaml / Pysch used by API server to parse yamlIn Progress07/09/2018

Associated revisions

Revision 77cfd1ee
Added by Peter Amstutz about 1 year ago

Merge branch '13681-wb-acr-submit-ram' closes #13681

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

History

#1 Updated by Peter Amstutz about 1 year ago

  • Status changed from New to In Progress

#2 Updated by Peter Amstutz about 1 year ago

  • Description updated (diff)

#3 Updated by Tom Morris about 1 year ago

  • Release set to 13

#4 Updated by Peter Amstutz about 1 year ago

  • Description updated (diff)
  • Status changed from In Progress to New
  • Release deleted (13)

#5 Updated by Peter Amstutz about 1 year ago

  • Release set to 14

#6 Updated by Peter Amstutz about 1 year ago

  • Release changed from 14 to 13

#7 Updated by Tom Clegg about 1 year ago

Is there an appropriate place to put this information in the workflow definition itself, rather than a separate column?

#8 Updated by Peter Amstutz about 1 year ago

Tom Clegg wrote:

Is there an appropriate place to put this information in the workflow definition itself, rather than a separate column?

Yes, I suppose we could add a new hint for it. Workbench would still need to know how to extract it in order to construct the container request.

#9 Updated by Tom Morris about 1 year ago

  • Target version changed from To Be Groomed to 2018-07-18 Sprint
  • Story points set to 1.0

#10 Updated by Peter Amstutz about 1 year ago

  • Assigned To set to Peter Amstutz

#11 Updated by Peter Amstutz about 1 year ago

13681-wb-acr-submit-ram @ 4e714090c2c81cddcef6da393b5680f3fcfe40b1

https://ci.curoverse.com/job/developer-run-tests/784/

  • New hint arv:WorkflowRunnerResources
  • Recognized by a-c-r Runner class and Workbench to set runtime_constraints for "ram" and "vcpus"
  • Providing --submit-runner-ram with --create/update-workflow will implicitly set arv:WorkflowRunnerResources in hints
  • Updated documentation

#12 Updated by Lucas Di Pentima about 1 year ago

Reviewing 334c069f4da11278844f4276b5c254e8cbf06412

  • There’re some sdk/cwl test failures on jenkins (already talked about this, I'm just recording the observation)
  • Just some code style observations on file sdk/cwl/arvados_cwl/arvworkflow.py
    • Line 55: Could that be replace with this?: hints = main.get(‘hints’, [])
    • The found var on line 57 could be replaced with an else clause on the for loop?

#13 Updated by Peter Amstutz about 1 year ago

Lucas Di Pentima wrote:

Reviewing 334c069f4da11278844f4276b5c254e8cbf06412

  • There’re some sdk/cwl test failures on jenkins (already talked about this, I'm just recording the observation)

Yea, I'm going to revert the change which is causing those failures, I filed #13766 to dig deeper.

  • Just some code style observations on file sdk/cwl/arvados_cwl/arvworkflow.py
    • Line 55: Could that be replace with this?: hints = main.get(‘hints’, [])

Yes, I don't know why I wrote it that way.

  • The found var on line 57 could be replaced with an else clause on the for loop?

No, because (according to my research) the "else" clause doesn't fire for empty arrays.

#15 Updated by Peter Amstutz about 1 year ago

  • Related to Bug #13766: [API] bug in libyaml / Pysch used by API server to parse yaml added

#16 Updated by Lucas Di Pentima about 1 year ago

Providing that tests pass OK, LGTM.

Regarding the else clause, it seems that they work even with empty lists:

Python 2.7.13 (default, Jun 27 2017, 10:00:24)
[GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> for a in []:
...  print "loop" 
... else:
...  print "else" 
...
else

#17 Updated by Peter Amstutz about 1 year ago

  • Status changed from New to Resolved
  • % Done changed from 75 to 100

Also available in: Atom PDF