Bug #4000

[API] "re-run with latest" yields fiddlesticks if pipeline template has been updated with new components

Added by Tim Pierce almost 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Tim Pierce
Category:
API
Target version:
Start date:
09/30/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0

Description

Attempting to re-run qr1hi-d1hrv-to0ukub4m8aqcg7 goes to
https://workbench.qr1hi.arvadosapi.com/pipeline_instances/qr1hi-d1hrv-to0ukub4m8aqcg7/copy?components=use_latest&pipeline_instance%5Bstate%5D=RunningOnServer&script=use_latest and produces:

Oh... fiddlesticks.
Sorry, I had some trouble handling your request.

undefined method `[]' for nil:NilClass

The cause here is that the pipeline template has been updated to include new components, so when PipelineInstancesController.copy attempts to copy the script_parameters for those components from the original instance, it throws an exception at https://arvados.org/projects/arvados/repository/revisions/9be9761666fd091496bee885b37598db9a6ab38a/entry/apps/workbench/app/controllers/pipeline_instances_controller.rb#L22:

srcvalue = source.components[cname][:script_parameters][pname]
More precisely: this occurs when copying a pipeline with components = use_latest, and
  • the pipeline template includes components that are not present in the instance
    • at least one of which includes a script_parameter that
      • is a hash
        • with a 'dataclass' field

Whew.

Short-term fix is to make sure that the copying process gracefully handles any unexpected changes between the template and the instance.


Subtasks

Task #4047: Review 4000-rerun-pipeline-changed-templateResolvedTim Pierce

Task #4021: make PipelineInstance.copy handle exceptions gracefullyResolvedTim Pierce

Associated revisions

Revision 3ee8ac51
Added by Tim Pierce almost 5 years ago

Merge branch '4000-rerun-pipeline-changed-template'

Closes #4000.

History

#1 Updated by Tim Pierce almost 5 years ago

  • Subject changed from "re-run with latest" button yields fiddlesticks to [API] "re-run with latest" button yields fiddlesticks
  • Description updated (diff)
  • Category set to API
  • Assigned To set to Tim Pierce

#2 Updated by Tim Pierce almost 5 years ago

  • Story points set to 1.0

#3 Updated by Tim Pierce almost 5 years ago

  • Description updated (diff)

#4 Updated by Tim Pierce almost 5 years ago

  • Subject changed from [API] "re-run with latest" button yields fiddlesticks to [API] "re-run with latest" yields fiddlesticks if pipeline template has been updated with new components
  • Description updated (diff)

#5 Updated by Tim Pierce almost 5 years ago

  • Description updated (diff)

#6 Updated by Brett Smith almost 5 years ago

Reviewing 5233f1d, and I have only one minor suggestion: it might be nice if the new tests checked that the copied pipeline's foo parameter was sourced from the original instance's. That would help ensure that we copied as much as possible from the instance, even while pulling in new components from the template.

But, that's not directly required to fix the bug, so feel free to merge with or without that change. Thanks.

#7 Updated by Tim Pierce almost 5 years ago

Brett Smith wrote:

Reviewing 5233f1d, and I have only one minor suggestion: it might be nice if the new tests checked that the copied pipeline's foo parameter was sourced from the original instance's. That would help ensure that we copied as much as possible from the instance, even while pulling in new components from the template.

But, that's not directly required to fix the bug, so feel free to merge with or without that change. Thanks.

Yes, I think it's a good idea to test that. In fact, adding those assertions to the test forced me to update the test fixtures, which didn't adequately exercise the change (which applies only to script parameters that are hashes with 'dataclass' fields).

Updated the test and the fixtures. Code remains the same. The new test fails if the code is reverted. Can you take another peek and confirm that we think this accurately tests the thing being fixed?

#8 Updated by Brett Smith almost 5 years ago

Tim Pierce wrote:

Updated the test and the fixtures. Code remains the same. The new test fails if the code is reverted. Can you take another peek and confirm that we think this accurately tests the thing being fixed?

I do, thanks. My only comment now is that it seems helpful to do these assertions in both new functional tests (the patch only adds them to the second). But again, that's relatively minor, and I'm happy to see a merge either way. Thanks.

#9 Updated by Tim Pierce almost 5 years ago

Brett Smith wrote:

Tim Pierce wrote:

Updated the test and the fixtures. Code remains the same. The new test fails if the code is reverted. Can you take another peek and confirm that we think this accurately tests the thing being fixed?

I do, thanks. My only comment now is that it seems helpful to do these assertions in both new functional tests (the patch only adds them to the second). But again, that's relatively minor, and I'm happy to see a merge either way. Thanks.

Thanks for the nudge on the other test. Updated it with the same assertions, and merged. Thanks.

#10 Updated by Tim Pierce almost 5 years ago

  • Status changed from New to Resolved

Applied in changeset arvados|commit:3ee8ac519f0c3f3fd211372d2a4699586d5c2aa8.

Also available in: Atom PDF