Arvados

Reviewing 6c40b57

A few notes:

• I tested it by running a crunch job that includes arvados_sdk_version and it ran successfully.
• Since #7582 it only runs the "pipe to perl" bootstrap script when arvados_sdk_version is specified in runtime_constraints. I think this means that "Crunch script run mode" can be streamlined for just that case. This also suggests an inability to install the SDK (such as missing virtualenv) should be a fatal error instead of a warning.
• Lines 965-970 are the simplified bash bootstrap script for when arvados_sdk_version is not specified; it would make sense to move pysdk_version_cmd to here (with errors suppressed, since this script is intended to work even if neither the Arvados SDK or even Python are available)

I'm not sure if this constitutes scope creep; the existing branch does solve the stated problem so I'd be willing to consider merging as-is.

Peter Amstutz wrote:

• Since #7582 it only runs the "pipe to perl" bootstrap script when arvados_sdk_version is specified in runtime_constraints. I think this means that "Crunch script run mode" can be streamlined for just that case. This also suggests an inability to install the SDK (such as missing virtualenv) should be a fatal error instead of a warning.

Technically, the Python SDK isn't the only SDK we have. The __DATA__ section makes at least some effort to install the Ruby and Perl SDKs as well, by extracting them and extending RUBYLIB and PERLLIB accordingly. It's arguable whether that's actually useful, but it's the current behavior, and that means that failing to install any one SDK is not fatal—maybe the Crunch script doesn't care about that specific language SDK. In turn, I don't think there's much opportunity to streamline this section.

I'm open to a discussion of narrowing the scope of this code, but because that's a bigger design discussion than this relatively straightforward bugfix, I'd rather handle it separately.

• Lines 965-970 are the simplified bash bootstrap script for when arvados_sdk_version is not specified; it would make sense to move pysdk_version_cmd to here (with errors suppressed, since this script is intended to work even if neither the Arvados SDK or even Python are available)

Added in 883322d. Thanks.

Brett Smith wrote:

Peter Amstutz wrote:

• Since #7582 it only runs the "pipe to perl" bootstrap script when arvados_sdk_version is specified in runtime_constraints. I think this means that "Crunch script run mode" can be streamlined for just that case. This also suggests an inability to install the SDK (such as missing virtualenv) should be a fatal error instead of a warning.

Technically, the Python SDK isn't the only SDK we have. The __DATA__ section makes at least some effort to install the Ruby and Perl SDKs as well, by extracting them and extending RUBYLIB and PERLLIB accordingly. It's arguable whether that's actually useful, but it's the current behavior, and that means that failing to install any one SDK is not fatal—maybe the Crunch script doesn't care about that specific language SDK. In turn, I don't think there's much opportunity to streamline this section.

I'm open to a discussion of narrowing the scope of this code, but because that's a bigger design discussion than this relatively straightforward bugfix, I'd rather handle it separately.

I agree.

• Lines 965-970 are the simplified bash bootstrap script for when arvados_sdk_version is not specified; it would make sense to move pysdk_version_cmd to here (with errors suppressed, since this script is intended to work even if neither the Arvados SDK or even Python are available)

Added in 883322d. Thanks.

Tested successfully. LGTM, please merge.