[WIP] Add workflow step parameters to workflow editor by mvdbeek · Pull Request #6829 · galaxyproject/galaxy

mvdbeek · 2018-10-08T10:03:31Z

This should allow specifying text, integer, float, boolean and color Parameters that can be re-used across workflow nodes. This build on 45962f3

mvdbeek · 2018-10-10T15:30:10Z

A good fraction of the UI is there and seems to work OK-ish, but the actual non-data parameter replacement only works for new-style workflow API submissions with inputs. The workflow run form still uses a different format for this. I'll see if it is sufficient to just move input_parameter to inputs, but IIRC there was a longer-term plan to unify UI and API. That isn't happening anytime soon, right ?

jmchilton · 2018-10-11T13:50:01Z

lib/galaxy/workflow/modules.py

        ],
    }

-    if trans.app.config.enable_beta_workflow_modules:


My two cents is that I'd keep the option and just promote parameter_input to an un-beta parameter.

jmchilton · 2018-10-11T13:51:03Z

I'll play with this and see if I can figure out an opinion on the API unification and fix the tests.

jxtx · 2018-10-11T15:44:21Z

Oooooh. Screenshots?

mvdbeek · 2018-10-11T15:55:00Z

Nothing fancy yet:

We should separate the data and non-data inputs in the workflow node (a bar, and maybe different color noodle), and the workflow inputs should move to the top where we also render the old workflow replacement parameters. And I'm sure there are plenty of bugs in there, just trying to figure this out.

jmchilton · 2018-10-12T14:02:23Z

The workflow run form still uses a different format for this. I'll see if it is sufficient to just move input_parameter to inputs, but IIRC there was a longer-term plan to unify UI and API. That isn't happening anytime soon, right ?

:( indeed it does. I'll start with the input parameters and see if I can separate them out, after that if I can separate out the data parameters so we are tracking them with database integrity using proper tracking with WorkflowRequestToInputDatasetAssociation / WorkflowRequestToInputDatasetCollectionAssociation the way standardized API calls with generate.

jmchilton · 2018-10-12T14:34:45Z

Working on ripping parameter inputs from the raw param map produced by the tool form and I encountered another problem - it looks like this branch breaks the parameter_inputs in some way. In particular the following test case that works in dev:

./run_tests.sh -api test/api/test_workflows.py::WorkflowsApiTestCase::test_run_with_text_connection

No longer works, the key line in the logs I think is seeing that there is an unmatched input connection.

galaxy.workflow.modules WARNING 2018-10-12 10:31:42,351 Failed to use input connections for inputs [set([u'seed_source|seed'])]

I'm going to see if I can figure out what caused this - none of the backend changes are obvious problems at first glance to me.

jmchilton · 2018-10-12T14:47:42Z

lib/galaxy/model/__init__.py

-        return (self.output_name == WorkflowStepConnection.NON_DATA_CONNECTION and
-                self.input_name == WorkflowStepConnection.NON_DATA_CONNECTION)
+        return (self.output_name == self.input_name == WorkflowStepConnection.NON_DATA_CONNECTION or
+                self.output_step and self.output_step.type == 'parameter_input')


Reverting this diff fixes the test case. I guess there aren't docs for this - but I don't like this change. The parameter_input is transferring data to the step - it isn't an implicit connection so this should return False IMO. I see why you made the change here - for when building the editor dict:

# FIXME: this removes connection without displaying a message currently! + input_connections = [conn for conn in input_connections if conn.input_name in data_input_names] - input_connections = [conn for conn in input_connections if conn.input_name in data_input_names or conn.non_data_connection]

Maybe we should bite the bullet and just not do that filtering at all - push all the connections to the editor? This way the editor can tell the user if there are invalid connections found and we can eliminate that FIXME?

Sounds good to me!

jmchilton · 2018-10-12T14:52:29Z

client/galaxy/scripts/mvc/workflow/workflow-view-terminals.js

+        var type = output.type;
+        var terminal = new Terminals.OutputParameterTerminal({
            element: this.el,
-            datatypes: type


Should these changes be reverted - given the class below?

Oh yeah, sure, should have cleaned this up before.

Going to remove this via rebase on this commit and force push to your branch - hope that is okay.

jmchilton · 2018-10-22T20:18:05Z

@mvdbeek any chance I can take this over this week and rebase #6776 underneath it? I'd like to add some Selenium tests and see if I can implement default values, but I can wait on you and limit the scope at how it stands now if you'd like.

mvdbeek · 2018-10-22T20:31:18Z

Sure, go ahead, happy to get this rolling!

jmchilton · 2018-10-24T12:33:11Z

@mvdbeek My new change is still creating more issues then solving and the GUI is terrible - but this is an outline of separating RuntimeValue into RuntimeValue and ConnectedValue (jmchilton@15e667c).

I guess my next step is to try to make data and non-data parameters more similar in terms of the tool state and module interface. The data parameters should have ConnectedValue as their tool state value and then we can uniformly have inputs with ConnectedValues appear with connections and eliminate the distinction between parameters and inputs. This will be good from an interface perspective but is also required for CWL where a parameter that can take an integer or a File for instance is totally possible. I'll probably merge #6911 into that to provide that consistent interface on the backend, I'll rework the frontend a bit - I think you have the right classes I just want to change how their instantiated a bit I think.

In the current state when a text, integer, float, boolean or boolean parameter is set as "Set at Runtime" a new connectable input appears for the tool node, to which a Parameter can be properly dragged and connected. This still needs a little more separation from data inputs/outputs, and the new connection isn't persisted in a meaningful way.

select lists don't take a `value` argument.

…dules I don't think anyone will start using them if they're hidden behing this flag.

…wfs.

…ditor.

mvdbeek added status/WIP area/UI-UX area/workflows labels Oct 8, 2018

mvdbeek force-pushed the workflow_step_parameters branch from 177a947 to e00fbbb Compare October 10, 2018 15:19

jmchilton reviewed Oct 11, 2018

View reviewed changes

jmchilton reviewed Oct 12, 2018

View reviewed changes

mvdbeek and others added 14 commits October 24, 2018 10:19

Allow saving workflows with input parameters

487e820

Allow inserting input parameter into workflow

33f7531

Fix rebuilding parameter node

2621f1c

Fix selecting the correct item from select list

d4c35ca

select lists don't take a `value` argument.

Fix getting correct type

d613db5

Fix restoring saved non-data connections in workflow editor

bee0896

Move Pause and Input Parameter modules out of enable_beta_workflow_mo…

f12f12a

…dules I don't think anyone will start using them if they're hidden behing this flag.

Fix node.addOuputParameter

7d83255

Don't render data inputs on parameter_input node

6b03716

More defensive runtime check

d5e21df

Allow loading in test-data workfows

f9a2e81

Add label to input parameters without labels.

41164e5

Give parameter inputs a workflow icon.

043698a

jmchilton added 2 commits October 24, 2018 10:20

Convert raw parameters from map into formal parameters when invoking …

2406ed6

…wfs.

Revert non-data connection handling, send connections unfiltered to e…

d2ab38a

…ditor.

jmchilton force-pushed the workflow_step_parameters branch from 75427fe to d2ab38a Compare October 24, 2018 14:21

This was referenced Oct 25, 2018

Add workflow step parameters to workflow editor - alternative. #6919

Closed

Add workflow step parameters to workflow editor (try 3) #6925

Merged

mvdbeek closed this Oct 26, 2018

Conversation

mvdbeek commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvdbeek commented Oct 10, 2018

Uh oh!

jmchilton Oct 11, 2018

Choose a reason for hiding this comment

Uh oh!

jmchilton commented Oct 11, 2018

Uh oh!

jxtx commented Oct 11, 2018

Uh oh!

mvdbeek commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmchilton commented Oct 12, 2018

Uh oh!

jmchilton commented Oct 12, 2018

Uh oh!

jmchilton Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

mvdbeek Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

jmchilton Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

mvdbeek Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmchilton Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

mvdbeek Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

jmchilton commented Oct 22, 2018

Uh oh!

mvdbeek commented Oct 22, 2018

Uh oh!

jmchilton commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mvdbeek commented Oct 8, 2018 •

edited

Loading

mvdbeek commented Oct 11, 2018 •

edited

Loading

mvdbeek Oct 12, 2018 •

edited

Loading