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

Introduce label and annotation attributes to subworkflows, fix annotation display, unify module form update handler #3593

Merged
merged 29 commits into from Feb 21, 2017

Conversation

Projects
None yet
4 participants
@guerler
Copy link
Contributor

commented Feb 10, 2017

This PR is another step forward in our effort to unify the workflow handling. ping @jmchilton.

  1. It generalizes label and annotation handling to include subworkflows

  2. It fixes the label and annotation display in the run workflow form

  3. Populate state is moved out of the tool class to the parameter handler, so it can be used like visit_inputs or other parameter related utilities

  4. Update state for all modules has been unified (see: https://github.com/galaxyproject/galaxy/pull/3593/files#diff-2fca1f4002bba50737993fd85ce5800fL358)

  5. Previous name handling has been reduced to data_input and data_collection_input modules as final preparation before stripping it in favor of the label attribute

  6. Adds unit tests for shared state populating procedure

@blankenberg blankenberg changed the title Strip name, replace with label, fix annotation display, unify module form update handler [WIP DEV] Strip name, replace with label, fix annotation display, unify module form update handler Feb 12, 2017

guerler added some commits Feb 15, 2017

@guerler guerler changed the title [WIP DEV] Strip name, replace with label, fix annotation display, unify module form update handler [WIP DEV] Introduce label and annotation attributes handler to subworkflowsl, fix annotation display, unify module form update handler Feb 16, 2017

@guerler guerler changed the title [WIP DEV] Introduce label and annotation attributes handler to subworkflowsl, fix annotation display, unify module form update handler [WIP DEV] Introduce label and annotation attributes to subworkflows, fix annotation display, unify module form update handler Feb 16, 2017

@guerler guerler added status/review and removed status/WIP labels Feb 16, 2017

@guerler guerler removed the status/WIP label Feb 16, 2017

@guerler guerler changed the title [WIP DEV] Introduce label and annotation attributes to subworkflows, fix annotation display, unify module form update handler Introduce label and annotation attributes to subworkflows, fix annotation display, unify module form update handler Feb 16, 2017

@@ -81,6 +81,9 @@ def get_type( self ):
def get_name( self ):
return self.name

def get_version( self ):
return None

This comment has been minimized.

Copy link
@jmchilton

jmchilton Feb 17, 2017

Member

There are some refactoring in here that look pretty nice to me.

But this one feels like more refactoring where a desire to treat everything in the frontend like "a tool" is causing poor design decisions on the backend. Version was a concept that I feel like only applied to tools - "Input Dataset" modules shouldn't have a get_version in the abstract unless that is something we plan to use - unless they have an inherent version.

Same with get_post_job_actions - it seems inappropriate that every module has a list of post job actions when only one kind of module even produces jobs.

This comment has been minimized.

Copy link
@guerler

guerler Feb 17, 2017

Author Contributor

Thanks for the quick review. Its so much appreciated. These placeholders are for the backend. The front-end does not change if a response attribute is set to None or not available at all. The alternative here would be to check if/else a module has get_version() or get_post_(job_)actions() before calling those instead of calling these placeholders. In the future I would like to rename get_post_job_actions to get_post_actions. My suggestion here is that its cleaner and better readable to use these placeholders instead of distributing if/else-statements through the code-base. It also comes with the benefit that these concepts are available for other modules if needed e.g. we could version subworkflows by the last date they were modified and then offer to update them when a workflow is loaded into the editor. Another application could be related to certain non-tool modules once their descriptions are moved to external yaml/xml-files.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Feb 21, 2017

Member

Regardless of how they are consumed - classes shouldn't in the abstract have inherited methods or attributes that don't make sense for their "type". If there are problematic if/else statements - that is an indication that perhaps a visitor pattern should be employed or functionality such as json-ification needs to move out of a manager/controller and into the module itself.

I wouldn't renamed get_post_job_actions to get_post_actions unless we have another module that actually has a post something or other action.

This comment has been minimized.

Copy link
@guerler

guerler Feb 22, 2017

Author Contributor

I totally agree, moving the jsonification out the controller into the module would be the right thing to do.

parameter_type = default_parameter_type
optional = default_optional

def get_inputs( self ):
# TODO: Use an external xml or yaml file to load the parameter definition
name = self.state.inputs.get( "name", self.default_name )

This comment has been minimized.

Copy link
@jmchilton

jmchilton Feb 17, 2017

Member

I really like the elimination of name here - thank you!

content_id = d["content_id"]
module.subworkflow = SubWorkflowModule.subworkflow_from_content_id( trans, content_id )
module.label = d.get( "label", None ) or None
from galaxy.managers.workflows import WorkflowsManager

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Feb 17, 2017

Member

I guess this import is here because of the circular import in lib/galaxy/managers/workflows.py:

from galaxy.workflow.modules import module_factory, is_tool_module_type, ToolModule, WorkflowModuleInjector

This comment has been minimized.

Copy link
@guerler

guerler Feb 17, 2017

Author Contributor

Yes. This import existed already before this PR. I just moved it here but I am still wondering if there is better way to do it.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Feb 21, 2017

Member

Yup - this is my hack, it definitely indicates a problem with the namespace design, and it needs to be fixed someday. I did prefer to tuck it away in a well named helper function to keep the main logic of that function simpler.

@guerler guerler force-pushed the guerler:unify_name_label branch from 604c79f to 8557c32 Feb 20, 2017

@jmchilton jmchilton merged commit cdb15f6 into galaxyproject:dev Feb 21, 2017

5 checks passed

api test Build finished. 263 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 140 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 24 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details
@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2017

Thanks a lot for the review.

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.