Story #9043

[Crunch2] [Workbench] Render CWL input selection forms

Added by Brett Smith over 4 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench
Target version:
Start date:
07/26/2016
Due date:
% Done:

67%

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

Description

CWL input form offer significantly more options than the existing Arvados pipeline template forms. Major points include arrays and union types (where more than one type of value is acceptable), which are characterized by the user needed to edit a list or select among different types which may offer different editing controls.

For now, we will adapt (or, if needed, copy) the existing Rails-based pipeline form so it can operate on a container request that has an embedded CWL workflow (as provided by #9766).

The first version must support:
  • text entry
  • number entry
  • file selection
  • directory selection (where the desired directory is an entire collection)
The first version might not support:
  • checkbox
  • control that wraps list of individual controls to add/reorder/remove items in arrays
  • control that wraps above controls to enable/disable optional parameters
  • directory selection (where the desired directory is not an entire collection)
exampleinputs.yml (899 Bytes) exampleinputs.yml Peter Amstutz, 07/13/2016 07:16 PM
inputs-post.json (2.39 KB) inputs-post.json Peter Amstutz, 07/13/2016 07:16 PM

Subtasks

Task #9806: Review branch 9043-edit-container-requestResolvedPeter Amstutz

Task #9805: [Workbench] Add a Run button to "show container request" page when state=UncommittedResolvedPeter Amstutz

Task #9860: Review branch 9043-test-edit-container-requestResolvedPeter Amstutz


Related issues

Related to Arvados - Story #9044: [Crunch2] [Workbench] Browse and show tools using the GA4GH tool registry APINew

Has duplicate Arvados - Feature #4605: [Workbench] Workbench support for generating input UI from common workflow tool description documents.Resolved03/07/2017

Associated revisions

Revision 7497b21c
Added by Peter Amstutz over 4 years ago

Merge branch 'origin-9043-test-edit-container-request' closes #9043

History

#1 Updated by Peter Amstutz over 4 years ago

  • Description updated (diff)

#2 Updated by Peter Amstutz over 4 years ago

  • Description updated (diff)

#3 Updated by Peter Amstutz over 4 years ago

  • Description updated (diff)

#4 Updated by Radhika Chippada over 4 years ago

Checked with Peter and one of the inputs can be a file from within an arvados collection and hence we need to be able to support this.

#5 Updated by Tom Morris over 4 years ago

#6 Updated by Brett Smith over 4 years ago

Stories:

  • Just render the workflow, no editing
  • Support editing non-compound input types
  • Support editing compound input types (this could be broken out into even more sub-stories, one per type, if need be)

#7 Updated by Peter Amstutz over 4 years ago

See attached exampleinputs.yml for something a user might write.

Normalize this file and get JSON by using cwltool --print-pre

cwltool --print-pre exampleinputs.yml > inputs-post.json

The attached inputs-post.json is an example of the normalized form. For UI it will be more robust to run the --print-pre normalization and load the resulting JSON.

We need the following editing controls:

  • text entry
  • number entry
  • checkbox
  • file selection
  • directory selection
  • control that wraps list of individual controls to add/reorder/remove items in arrays
  • control that wraps above controls to enable/disable optional parameters

#8 Updated by Tom Clegg over 4 years ago

  • Category set to Workbench
  • Assigned To set to Tom Clegg

#10 Updated by Tom Morris over 4 years ago

  • Target version set to Arvados Future Sprints

#11 Updated by Tom Clegg over 4 years ago

  • Description updated (diff)

#12 Updated by Tom Morris over 4 years ago

  • Story points set to 4.0

#13 Updated by Peter Amstutz over 4 years ago

  • Story points deleted (4.0)

Relevant bits of existing field component rendering:

workbench/app/views/pipeline_instances/_show_components_editable.rb
Render the pipeline template form entry by going through each element in the script parameter of the pipeline template and calling render_pipeline_component_attribute

      <% component[:script_parameters].andand.each do |p, tv| %>
        <tr>
          <td style="border-top: none"></td>
          <td style="border-top: none"></td>

          <td class="property-edit-row"><%= p %></td>
          <td class="property-edit-row"><%= 
render_pipeline_component_attribute (editable && @object), 
  :components, [k, :script_parameters, p.to_sym], tv %></td>
        </tr>
      <% end %>

workbench/app/helpers/application_helper.rb
Renders HTML for a single editable field (with AJAX update).
May need to be tweaked/updated for CWL fields.

  def render_pipeline_component_attribute(object, attr, subattr, value_info, htmloptions={})

workbench/app/controllers/pipeline_instances_controller.rb
Accepts AJAX PUT from browser to update a field (the corresponding server side for above code).
Will need to be copied in some form to container_requests:

  def update
    @updates ||= params[@object.class.to_s.underscore.singularize.to_sym]
    if (components = @updates[:components])
      components.each do |cname, component|
        if component[:script_parameters]
          component[:script_parameters].each do |param, value_info|
...
end

#14 Updated by Peter Amstutz over 4 years ago

  • Story points set to 4.0

#15 Updated by Tom Morris over 4 years ago

  • Assigned To changed from Tom Clegg to Peter Amstutz
  • Target version changed from Arvados Future Sprints to 2016-08-31 sprint

#16 Updated by Radhika Chippada over 4 years ago

  • Status changed from New to In Progress

#17 Updated by Radhika Chippada over 4 years ago

Comments regarding 7700e66

- container_requests_controller.rb => show_pane_list

  • This “if @object and @object.state 'Uncommitted'” can be “if @object.andand.state 'Uncommitted’”
  • Should the “Graph” tab also be hidden when Log is hidden?
  • Fiddlesticks when I visit the Graph tab

- container_requests_controller.rb => update

  • Since most of the code is in “if input_obj” block, it might be better if you unpyramid the code and return earlier if input_obj is not found?
  • same with “if input_obj.include? input_schema[:id]” - use a continue if not?
  • In “c = display_value = Collection.find(re1)” : it appears that display_value is unused?
  • Would you be able to define “/^([0-9a-z]{5}-([0-9a-z]{5})-[0-9a-z]{15})(\/.*)?$/“ once and reuse than repeat it?

- application_helper

  • Instead of “null” can we use “optional” here to indicate an optional parameter? — required = !(input_schema[:type].include? "null")
  • It appears that “id” in “inputs” is what distinguishes one from the other. If I had something as below for my inputs definition in the workflow, it still is allowed and rendered but it appears that populating it may be problematic. Do we need to some kind of validation that an id is not repeated in the workflow?
                    "inputs": [
                        {
                            "type": "double",
                            "id": "ex_double",
                            "inputBinding": {"position": 1}
                        },
                        {
                            "type": "double",
                            "id": "ex_double",
                            "inputBinding": {"position": 1}
                        }
    
  • When I select “false” for a boolean input field, it’s being shown in UI as “#”

- container_requests_test

  • Does “INPUT_SELECTOR” need to be all uppercase?
  • The test of course is not passing, but I think you are aware of it already.

#18 Updated by Peter Amstutz over 4 years ago

Radhika Chippada wrote:

Comments regarding 7700e66

- container_requests_controller.rb => show_pane_list

  • This “if @object and @object.state 'Uncommitted'” can be “if @object.andand.state 'Uncommitted’”

Done.

  • Should the “Graph” tab also be hidden when Log is hidden?
  • Fiddlesticks when I visit the Graph tab

The Graph tab doesn't exist, I got rid of it.

- container_requests_controller.rb => update

  • Since most of the code is in “if input_obj” block, it might be better if you unpyramid the code and return earlier if input_obj is not found?

It calls "super" at the end so I prefer not to change the flow.

  • same with “if input_obj.include? input_schema[:id]” - use a continue if not?

Done.

  • In “c = display_value = Collection.find(re1)” : it appears that display_value is unused?

Fixed.

  • Would you be able to define “/^([0-9a-z]{5}-([0-9a-z]{5})-[0-9a-z]{15})(\/.*)?$/“ once and reuse than repeat it?

You're right, there's already a helper function for this, I changed it to use the helper.

- application_helper

  • Instead of “null” can we use “optional” here to indicate an optional parameter? — required = !(input_schema[:type].include? "null")

No, "null" is how CWL represents optional inputs (an optional input is defined as one where "null" is a permitted value).

  • It appears that “id” in “inputs” is what distinguishes one from the other. If I had something as below for my inputs definition in the workflow, it still is allowed and rendered but it appears that populating it may be problematic. Do we need to some kind of validation that an id is not repeated in the workflow?
    [...]

That's an error in the workflow, I'm not sure where the validation should happen.

  • When I select “false” for a boolean input field, it’s being shown in UI as “#”

Ok, I think I fixed it. I keep having trouble because both Ruby and Javascript have notions about "false == null".

- container_requests_test

  • Does “INPUT_SELECTOR” need to be all uppercase?

I copied that from the pipeline_instances test.

  • The test of course is not passing, but I think you are aware of it already.

Yes, for some reason it's not able to find the "ok" button to submit the value, I haven't looked into it.

#19 Updated by Radhika Chippada over 4 years ago

Thanks for addressing those.

  • There is still one more occurrence of /^([0-9a-z]{5}-([0-9a-z]{5})-[0-9a-z]{15})(\/.*)?$/ in application_helper
  • Regarding "That's an error in the workflow, I'm not sure where the validation should happen" : I think it would be desirable that we add this validation to workflow objects. In any case, would you please add ticket about this for future consideration? Thanks.

#20 Updated by Peter Amstutz over 4 years ago

Radhika Chippada wrote:

Thanks for addressing those.

  • There is still one more occurrence of /^([0-9a-z]{5}-([0-9a-z]{5})-[0-9a-z]{15})(\/.*)?$/ in application_helper

Fixed, thanks.

  • Regarding "That's an error in the workflow, I'm not sure where the validation should happen" : I think it would be desirable that we add this validation to workflow objects. In any case, would you please add ticket about this for future consideration? Thanks.

This validation should really happen as part of CWL document loading & validation, I filed #9853 which has a link to a schema salad issue.

#21 Updated by Radhika Chippada over 4 years ago

Branch 9043-test-edit-container-request at commit e93aaa7

  • Updated application_helper to address the issue where “number” was not allowing decimal numbers to be entered for floats and doubles. Set to text in these cases
  • added tests to add and verify input for double, float, int, int_opt, long, string, string_opt, boolean, enum, directory, file typed inputs
  • added test to verify that the Run button is enabled and works as expected when all required inputs are provided

#22 Updated by Radhika Chippada over 4 years ago

Couple more comments / questions about containers_request.yml updates at 3bdb80d

  • Both uncommitted and uncommitted_ready_to_run have ex_opt_string and ex_string_opt. Can we remove the second one in these fixtures?
  • Since we do not yet support array types, do we want to remove ex_opt_file_array from the fixtures?

#23 Updated by Peter Amstutz over 4 years ago

Radhika Chippada wrote:

Couple more comments / questions about containers_request.yml updates at 3bdb80d

  • Both uncommitted and uncommitted_ready_to_run have ex_opt_string and ex_string_opt. Can we remove the second one in these fixtures?

Yes, that was a mistake, I think I added the 2nd optional string not realizing the first one was already there.

  • Since we do not yet support array types, do we want to remove ex_opt_file_array from the fixtures?

Yes.

#24 Updated by Peter Amstutz over 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:7497b21c937bb6e8451f16047945b7cfc9081a53.

Also available in: Atom PDF