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

Mako to JavaScript views for Workflow's Run and Import actions #4169

Merged
merged 24 commits into from Jun 21, 2017

Conversation

Projects
None yet
6 participants
@anuprulez
Copy link
Member

commented Jun 7, 2017

This PR reconstructs views for Run and Import Workflow actions in JavaScript and remove their respective mako files.

screenshot from 2017-06-07 22 10 38

Import action opens the view in the middle panel. This work is in continuation of these two pull requests to transform Workflow's mako views to JavaScript views:

Apart from these actions, the PR also moves the link to open Configure Workflow view to User Preferences. The Api calls to update the workflow items list in the Tool Panel remains with the Workflow's Api.

config-user

Thanks a lot to @bgruening

@guerler Thanks a lot for your help in fixing the bug related to the subsequent load of tools, datasets with a huge workflow (50+ steps) loaded in the center panel.

All suggestions are welcome!

@galaxybot galaxybot added the triage label Jun 7, 2017

@galaxybot galaxybot added this to the 17.09 milestone Jun 7, 2017

@anuprulez anuprulez changed the title Mako to JavaScript views for Worflow's Run and Import actions Mako to JavaScript views for Workflow's Run and Import actions Jun 7, 2017

var self = this,
id = get_querystring( 'id' ),
url = Galaxy.root + 'workflow/run_workflow?id=' + id;
$.getJSON(url, function( response ) {

This comment has been minimized.

Copy link
@guerler

guerler Jun 8, 2017

Contributor

I suggest to remove the 'Run_Workflow_View' module and just use 'Utils.get' in the 'show_run' function above, and then directly load the 'ToolForm.View'. Use that route to load the run workflow form from other locations as needed.

}
});

var Import_Workflow_View = Backbone.View.extend({

This comment has been minimized.

Copy link
@guerler

guerler Jun 8, 2017

Contributor

Rename to 'ImportWorkflowView' for consistency with other modules.

$.getJSON(url, function( response ) {
var wf_parsed = JSON.parse( JSON.stringify( response ) );
var form = new ToolForm.View( wf_parsed[0] );
$( '#galaxy_main' ).hide();

This comment has been minimized.

Copy link
@guerler

guerler Jun 8, 2017

Contributor

These global selectors should not be used. Use the 'page' object instead, but in this particular case here I suggest to redirect using the route in the analysis app, instead of manually loading the tool form.

_registerWorkflowMenuClick: function( self, workflow_id ) {
$( '.workflow-menu-' + workflow_id ).click(function( e ) {
var url = Galaxy.root + 'workflow/run_workflow?id=' + workflow_id;
$.getJSON(url, function( response ) {

This comment has been minimized.

Copy link
@guerler

guerler Jun 8, 2017

Contributor

Using 'Utils.get' can reduce overhead, making stringify and parse below unnecessary.

redirect_url = url_for( '/' ) + 'workflow?status=' + status + '&message=%s' % escape( message )
return trans.response.send_redirect( redirect_url )
else:
return json.dumps({

This comment has been minimized.

Copy link
@guerler

guerler Jun 8, 2017

Contributor

Is json.dumps here necessary? You can use '@web.json' instead.

return trans.fill_template( 'workflow/run.mako', workflow_dict=workflow_dict )
wf_dict = list()
wf_dict.append( workflow_dict )
return json.dumps( wf_dict )

This comment has been minimized.

Copy link
@guerler

guerler Jun 8, 2017

Contributor

Should be just 'return workflow_dict'. If there is an api function which already does this we should remove this.

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2017

@guerler Thanks a lot for the review!
The usage of @web.json in API calls removed the need of stringifying and parsing the JSON again at the client side.

@@ -948,7 +954,8 @@ def build_from_current_history( self, trans, job_ids=None, dataset_ids=None, dat
url_for( controller='workflow', action='run', id=workflow_id ) ) )

@web.expose
def run( self, trans, id, history_id=None, **kwargs ):

This comment has been minimized.

Copy link
@guerler

guerler Jun 9, 2017

Contributor

There is a 'workflow_dict' function in the API which looks very similar, maybe we can use that one and just remove this one here.

This comment has been minimized.

Copy link
@anuprulez

anuprulez Jun 9, 2017

Author Member

I tried using the 'workflow_dict' method in API to pull the workflow's JSON but it looks a bit different. And using it makes the workflow steps undefined in the run mode. I think we need to merge the calls. I will look into it more.

Workflow dict using controller call ('run_workflow') (which works perfectly in run mode):

controller_wf

Workflow dict using api call ('workflow_dict'):

api_wf

This comment has been minimized.

Copy link
@guerler

guerler Jun 9, 2017

Contributor

There is a style parameter which has to be set to 'run' I think.

url : Galaxy.root + 'workflow/run_workflow?id=' + Utils.getQueryString( 'id' ),
success : function( response ) {
var form = new ToolFormComposite.View( response );
self.page.$( "#center-panel" ).empty().append( form.$el )

This comment has been minimized.

Copy link
@guerler

guerler Jun 9, 2017

Contributor

Can you explain why using 'this.page.display' is not an option here?

This comment has been minimized.

Copy link
@anuprulez

anuprulez Jun 9, 2017

Author Member

It works. I am not sure how it would when we need to show an error in the async call.

This comment has been minimized.

Copy link
@guerler

guerler Jun 9, 2017

Contributor

Maybe you could just show an 'alert' if the request fails or use display to load a large Ui.Message view.

'url' : url,
'message' : message,
'status' : status,
'myexperiment_target_url' : myexperiment_target_url

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jun 9, 2017

Member

FYI, spaces before a colon : are discouraged. We don't enforce E203 in our style guidelines, but only because we allow to align the dictionary values.

Not a merge blocker anyway.

This comment has been minimized.

Copy link
@anuprulez

anuprulez Jun 9, 2017

Author Member

thank you! I will remove the extra spaces before colons.

anuprulez added some commits Jun 10, 2017

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2017

@guerler does it look ok?

@guerler

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2017

Yeah this looks great to me. Can you do a performance test by checking how the UI behaves if you load the run workflow form multiple times with workflows containing 50+ steps and without reloading the entire page? Does the UI slow down? If not I think this is good to go.

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2017

@guerler I looked into it and have a couple of observations. I created a workflow with 60+ steps and made 3-4 copies.

copy50 workflow

I am able to load run workflow multiple times as each load of run workflow (for different workflow) automatically refreshes the page. But, when I try to load any tool from the left panel with 60+ steps workflow already loaded in the center panel, I get an unresponsive script error which does not happen if I load any tool (like AXT_TO_LAV) with a smaller workflow (say with 10 steps) already loaded in the center panel.

This error I get:

unresponsive

@guerler

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2017

That is interesting. We need to figure out why this happens.

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2017

I am looking into the issue. One difference we have now is that we are displaying run workflow in the center-panel section of the Galaxy's middle part and just the needed div. Before this change, they were displayed in the iframe (galaxy_main) as complete html.
It seems to be some issue with the switching between the iframe and center-panel of the middle sections. In the layout/panel.js, there is a method _iframeChangeHandler and inside it, this.$( '#center-panel' ).hide(); does not work (because the center panel does not hide and even the url corresponding to the tool clicked appears in the url box of the browser) when we try to open a tool while there is already a huge workflow opened in the center panel. It goes for long, unresponsive script.

The selected tool's form loads but in about 2 mins which is unacceptable. The same issue is there with history items. May be all the links which open in the iframe.

Update: When a huge workflow is loaded in the center panel and any tool is clicked, the following line in /mvc/too/tool.js takes a lot of time to pull the tool's resource:
Galaxy.router.push( '/', { tool_id : self.model.id, version : self.model.get('version') } );

By this way, eventually that tool gets loaded in the galaxy_main (iframe) but after a long time (~2 minutes).

When I replaced this line by:
window.location = Galaxy.root + '?tool_id=' + self.model.id + '&version=' + self.model.get('version');

the tools gets loaded instantly but the page refreshes and it is loaded into the center-panel and not galaxy_main.

Do you think this kind of solution is feasible or we should try to open the run workflow in the iframe itself? Because previously, run workflow was opening in the iframe. There did not seem to be any problem even with huge workflow.

@guerler

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2017

So I think I identified the underlying issue. I am working on a fix now.

@bgruening

This comment has been minimized.

Copy link
Member

commented Jun 19, 2017

@guerler is it something with the routing?

@guerler

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2017

No its related to how objects get destroyed. We have a mechanism to make sure that deferred objects are probably destroyed but it seems to have flaw in this particular use case.

@guerler

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2017

I issued a PR against this branch to fix this issue: bgruening#36. Thanks a lot for working on this.

Merge pull request #36 from guerler/fix_cleanup
Fixes deferred removal of previous views, augments center panel handling
@bgruening

This comment has been minimized.

Copy link
Member

commented Jun 19, 2017

@anuprulez could you test if this fixes your problem as well?

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2017

@guerler With your latest changes, it works fine. The issue I reported is fixed. Thanks a lot for your help :)

@bgruening

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

@anuprulez we need again a merge with master here :( Sorry.
@dannon is this then ready to go? Not sure if Sam is allowed to merge this one now that he has contributed to it :)

@dannon

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

@bgruening @anuprulez No need for another merge, I'll go through this and merge locally in a second.

@bgruening

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

Nice! Step by step we are killing the makos!

@dannon dannon merged commit 8a09836 into galaxyproject:dev Jun 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.