Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalizes the handling of steps and missing tools for the workflow editor #3181

Merged
merged 92 commits into from Jan 24, 2017

Conversation

Projects
None yet
8 participants
@guerler
Copy link
Contributor

commented Nov 16, 2016

This PR unifies the handling and storage of workflow modules and their states. Now, preruntime and runtime states are aligned with the front-end form rendering. The overall goal of this PR is to streamline the module handling process.

Please see also comments below within this thread.

Here is a list of changes so far:

  1. All modules are handled through a single base class which uniformly encodes and decodes preruntime and runtime states i.e. the SimpleWorkflowModule layer for non-tool modules has been removed.

  2. Preruntime states for non-tool modules specify the same input parameter types as tool modules i.e. we could provide xml or yaml descriptions for non-tool module input parameters similar to tool.xmls.

  3. Tool and node input errors are not stored or exported anymore. We evaluate them when module states are injected.

  4. Datalists which provide suggestions for collection input types are handled through the same process as regular tool input parameters i.e. the TextToolParameter has been augmented. The front-end implementation for displaying these options is not part of this PR but will be implemented in a follow up.

  5. All workflow modules return a form object instead of html. The form object contains input definitions which are rendered through the regular js-form view. The editor form mako for non-tool makos has been removed. We also removed the corresponding front-end view for non-tool modules EditorFormView.

  6. If no workflow node is selected in the editor view, the global workflow options form is shown instead of the no node selected notification message.

  7. Missing tools are handled like regular tools without the option to edit parameters or to save the workflow.

  8. Several functions have been removed from the module handling process as a result of the unification such as the separate get_new_module_info (uses build_module instead), editor_form_post (uses build_module instead), new (uses from_dict instead), update_state (uses recover_state instead), get_runtime_input_dicts, _abstract_config_form, default_state callers.

  9. For backward compatibility, modules states are loaded using safe_loads instead of loads. Once stored, workflow module states are upgraded to the unified state format.

ping @jmchilton

@martenson

This comment has been minimized.

Copy link
Member

commented Nov 16, 2016

xref #3064 #2823

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2016

Thanks @martenson

@guerler guerler changed the title Normalizes the handling of steps and missing tools for the workflow editor [WIP] Normalizes the handling of steps and missing tools for the workflow editor Nov 21, 2016

@guerler guerler force-pushed the guerler:fix_workflow_issues branch from 393dd38 to 6905f5b Dec 1, 2016

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2017

@jmchilton Thank you for your excellent review. I am aware of the ui regressions and would work instantly after this PR on it if thats ok with you. It should be straightforward and ready before the next release and this PR is already fairly complex. Regarding handling all module states as tool states, it is something I thought quite a bit about. We have four states of which only one, the non-tool module preruntime state is handled differently and that produces significant overhead and layers. Non-tool module runtime states are already handled as tool states and I believe that regular tool states are more powerful on the long run which is why I aligned this 1-out-of-4 handling mechanism.

@erasche

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

A suggestion box but allowing for other inputs? I would use that

  • jbrowse tool allows choosing from GFF3 fields, which should be displayed as a feature label (e.g. can display ID field or Name field). Right now it's a plain text box, would be better with suggestions.
  • jbrowse allows for selecting an icon to represent certain menus, the list is gigantic but showing just a handful of popular ones would be nice, and allowing people to use the full list only by browsing the documentation.
@jmchilton

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

@guerler You could put it that way or you could say 5 modules types had simple forms for configuration before this PR and now all 6 module types are defining complex tool parameters and state for configuration.

I really do not want this PR to be merged with that regression. How about we make a deal? I will rescind the -1 however if you promise that it is fixed before "the next release" and if it isn't you do the work of rolling it back this PR before "the next release" is announced.

@erasche Fair enough - that is all I need. I'll add an example of this to the example tools and update the language in the XSD via a PR to this branch.

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2017

Yeah, I should need just 2-3 days to fix those two ui regressions. So this week it should be good to go.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

@guerler I'm doing some testing on the new tool stuff and I realized actually the drop down for the collection type doesn't appear anymore either - is that known?

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2017

Yes. I want to implement something which also works for Safari, also before the release. If I dont find a solution which also works for Safari, I will just use regular HTML5 datalists again. Its mentioned in 4. in the above list of changes.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jan 9, 2017

@guerler I opened a PR for what I want to see in the tool stuff and to tweak a logging level guerler#17.

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2017

Thanks @jmchilton

@gregvonkuster

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2017

Just wondering: if this PR is merged, will anything be broken on the dev branch? I'm wondering due to the regression fixes, missing features like select lists, etc that will be done before "the next release" is announced, which I assume is a ways out. I have several systems that I'm running from the dev branch, but I'll hold off on updating them if this PR will pose problems for workflows, etc. ;)

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2017

It should not cause any issues but it would be great if you can run tests. The regressions are very minor ui regressions related to displaying icons, editing the label in the title of the form instead of the text field used in this PR and HTML5 datalists which are not supported in Safari and only used to display suggestions when editing the collection type text field.

@gregvonkuster

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2017

Ok, thanks for the info @guerler ;)

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2017

@jmchilton we can also merge this after branching 17.01 if you think thats better.

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

@guerler I think it would be better - I will try to do what I can to ensure we branch 17.01 as soon as possible.

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2017

Sounds good. Thanks a lot.

@jmchilton jmchilton modified the milestones: 17.05, 17.01 Jan 12, 2017

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

Switched milestone in Github to reflect this.

guerler added some commits Jan 14, 2017

@jmchilton jmchilton merged commit 3c4413d into galaxyproject:dev Jan 24, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
api test Build finished. 256 tests run, 0 skipped, 0 failed.
Details
framework test Build finished. 137 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

Merged with reservation previously discussed but very thankful to @guerler for waiting so long on this.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Jan 27, 2017

martenson added a commit that referenced this pull request Jan 27, 2017

Merge pull request #3504 from jmchilton/selenium_fixes_4
Fix selenium workflow tests broken with #3181.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.