Arvados

6264-run-command-task-env @ 15ccbba is good to merge.

One thing I noticed is that if the user specifies a non-string value in task.env (most likely a numeric type), they'll get an apparently random exception from subst.do_substitution that doesn't really enlighten what the problem is. This same issue exists elsewhere in the code, but this sort of accidental type error seems more likely for task.env than, say, task.stdout. I wanted to flag it, but if you think dealing with it is out of scope for the branch, that's fair.

Thanks.

Reviewing 6264-cwl-runner @ 53755b6

Considering the circumstances, I think this is good to merge. I would like to see tests, but I figure that's not doable before BOSC. I know you've put a lot of work into manual testing—maybe a quick demo or tour of how that works on Monday would be good.

Suggestions I'd make from here:

• I think I'd have had an easier time following the code if these variables had more descriptive names:
  • i in _match
  • t and r in arvExecutor
  • (I know this isn't new code, but) n and c in uploadfiles
• Putting the reuse command line switches in a mutually exclusive group would make a nice user affordance.
• I think the three-line for loop to copy self.environment to task.env can just become script_paramters["task_env"].update(self.environment).
• Why does the regexp in ArvPathMapper capture the path (in a group, natch)? I don't see it doing anything with that, so it looks like needless complexity.

Thanks.

Brett Smith wrote:

Reviewing 6264-cwl-runner @ 53755b6

Considering the circumstances, I think this is good to merge. I would like to see tests, but I figure that's not doable before BOSC. I know you've put a lot of work into manual testing—maybe a quick demo or tour of how that works on Monday would be good.

Suggestions I'd make from here:

• I think I'd have had an easier time following the code if these variables had more descriptive names:
  • i in _match
  • t and r in arvExecutor

Fixed.

• (I know this isn't new code, but) n and c in uploadfiles

Didn't fix.

• Putting the reuse command line switches in a mutually exclusive group would make a nice user affordance.

Good idea, done.

• I think the three-line for loop to copy self.environment to task.env can just become script_paramters["task_env"].update(self.environment).

Thanks, done.

• Why does the regexp in ArvPathMapper capture the path (in a group, natch)? I don't see it doing anything with that, so it looks like needless complexity.

Can't remember, either that's was leftover from copying the regex from somewhere else (where unfortunately the source regex wasn't exactly what I needed...) or I was using it and then refactored the part that needed the path somewhere else. In either case, I removed the capture group.

Thanks

Peter Amstutz wrote:

Brett Smith wrote:

• Why does the regexp in ArvPathMapper capture the path (in a group, natch)? I don't see it doing anything with that, so it looks like needless complexity.

Can't remember, either that's was leftover from copying the regex from somewhere else (where unfortunately the source regex wasn't exactly what I needed...) or I was using it and then refactored the part that needed the path somewhere else. In either case, I removed the capture group.

But what I'm saying is, why match the path characters at all? When all you're doing is a boolean match, ^string.* is functionally identical to ^string.

But this is still good to merge, thanks.