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

Run workflow form revision #2077

Merged
merged 1 commit into from Jul 7, 2016

Conversation

Projects
None yet
6 participants
@guerler
Copy link
Contributor

commented Apr 4, 2016

In order to reliably replicate the behavior of the current run workflow form, this PR contains a minimal, controller-consistent workflow api endpoint. It would be fine to remove this endpoint within this PR and redirect the client, once we understand why currently existing api endpoints and legacy variations are unable to replicate the basic behavior expressed by the current workflow controller endpoint. I think that the presented endpoint can assist to understand the differences.

Please set the galaxy.ini flag run_workflow_toolform_upgrade to True in order to activate the new run workflow.

@guerler guerler force-pushed the guerler:revise_workflow_run_004 branch 7 times, most recently from 3ac416a to 07a377d Apr 4, 2016

@guerler guerler force-pushed the guerler:revise_workflow_run_004 branch 18 times, most recently from 997c1f3 to d554895 Apr 6, 2016

@guerler guerler force-pushed the guerler:revise_workflow_run_004 branch from bee162d to ddc8acc Jun 9, 2016

@guerler guerler force-pushed the guerler:revise_workflow_run_004 branch from ddc8acc to 924ceab Jun 20, 2016

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2016

Thanks for the detailed response. Yes, please fix the invocation endpoint to accept these and add any other regressed feature to existing API endpoints such that they are consistent with the existing controller endpoint e.g. thanks for adopting the missing linked and unlinked batch mode features which were re-implemented in this PR by merging #2464. Without this feature users would have not been able to use any batch mode options with the new run workflow form. Correct me if I am wrong, but I noticed another feature which is not supported by existing API endpoints but by the endpoint presented here which is that data input modules should be able to submit multiple selections if associated with a multiple input data parameter. Now, the new API endpoint, which is the focus of this discussion and which is blocking the introduction of the new run workflow form, contains only about 50 lines.

@guerler guerler force-pushed the guerler:revise_workflow_run_004 branch from 924ceab to 34b3749 Jun 20, 2016

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jun 28, 2016

@guerler I'm disappointed, but if you fix the conflicts I will merge the PR.

@guerler guerler force-pushed the guerler:revise_workflow_run_004 branch from 34b3749 to 89b8388 Jul 5, 2016

@galaxybot galaxybot added this to the 16.07 milestone Jul 5, 2016

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 7, 2016

I'm a strong -.9 on this PR - it is a real step backward for running workflows in Galaxy. I spent months attempting to centralize and de-duplicate code for executing workflows and implementing a invocation-centric back-end for workflows that had some hope for interesting scheduling, features, and scaling.

@jmchilton jmchilton merged commit 83f1b07 into galaxyproject:dev Jul 7, 2016

4 checks passed

api test Build finished. 219 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 110 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 582 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 15, 2016

@guerler I've chatted with a few people about this and been thinking about it myself - I have gotten the sense that perhaps you were just eager to get this done with so you can move on the tool form work? I'm not eager to take up the UI work needed to do this well - but if you are eager to just move on I'd rather do the work myself (or with the help of @martenson) than let this PR into a Galaxy release in this form.

If over the next few months, I take over the form work you started would you be willing to revert this PR? Would this free you up to work on something you feel more passionate about - such as viz?

@dannon

This comment has been minimized.

Copy link
Member

commented Jul 18, 2016

I don't want to speak for @guerler here, but it looks like he's tried to make this work with existing endpoints for quite some time before going this route. In my opinion backing it out of the release isn't necessary if this serves a useful (if temporary, as specified in the PR summary) stepping stone for his work on the workflow editor.

Would adding words of warning to the endpoint documentation stating that it's not expected to be a permanent API addition, etc, be a reasonable compromise?

Temporary API endpoint to support new workflow editor development -- this endpoint may disappear at any time, continue to use other endpoints for external API consumption.

Or, perhaps change the route name as well?

api/workflows/beta_run

There's a precedent for this sort of work being in releases in the history contents API and the handling of the 'dev' key, which swaps to index_v2, below, with different options and behavior. https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/webapps/galaxy/api/history_contents.py#L470

@jxtx

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2016

What about moving it to a location/route that is not under /api, to be clear it is not a supported stable API, and then remove it once the UI work to support the new endpoint is complete?

@dannon

This comment has been minimized.

Copy link
Member

commented Jul 18, 2016

@jxtx I like that, too.

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2016

The issue is not really UI related, the problem is that the newer endpoints come with better scheduling behavior which we would like to benefit from but have skipped existing features to be fixed by others such as both batch mode options, data inputs within modules and proper history refreshing. Claiming that the newer endpoints are backward compatible or that they were just not used right without specifying how to use them, even within this thread, has caused severe delays and literally blocked our efforts to replace the existing workflow form and to work on the real challenges related to the new run workflow form i.e. performance/load times. It took till June 1 to accept that at least certain features are missing i.e. The implementation here is okay and could probably be adapted in the invocation API. Although it was not part of this project, we had to demonstrate that an endpoint containing about ~50 lines of code is actually capable of replicating the currently existing behavior in order to remove the block placed on our efforts. Until today almost every workflow has been executed like this. In that sense this endpoint is not backward but current. I again suggest to integrate the dropped features into the newer endpoints and document their usage. It is trivial to redirect or adjust the workflow submission data to any other functional endpoint.

@martenson

This comment has been minimized.

Copy link
Member

commented Jul 18, 2016

@guerler I agree the UI is not the challenge here and I also support @jxtx with moving the new (temporary) endpoint to something like /beta with the strong reason of making the API consistent, well-defined and stable. Hopefully by 16.10 we will be able to consolidate these endpoints into a uniform API that will strongly support both @guerler's and @jmchilton's efforts.

@jxtx

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2016

Maybe not beta but something like private or internal to make it clear noone should rely on it staying or becoming stable.

@martenson

This comment has been minimized.

Copy link
Member

commented Jul 18, 2016

xref #2622

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 18, 2016

There is no "regression' in the workflow invocation API - I don't like this framing and I don't think it is at all appropriate. The invocations endpoint was initially designed to replace the old workflow run API - and it is feature complete with respect to that - or at least the portion of that which has tests and which I understand. The act of replacing the workflow run form to target the API instead of a controller method is work undertook by @guerler and I guess is the scope of this PR. I don't think it is at all fair to claim I "skipped existing features to be fixed by others" - it is work I'd be willing to do, eager at this point, but we are going to ahead with a new API endpoint instead of continuing work on the existing API endpoint.

As for the 3 "regressions":

  • I tried to engage in a conversation about the non-input module data inputs and there was no counter point made - it was an explicit design decision that three of us made in the past not to support them. I offered alternatives, I'd be willing to have a conversation about updating the data model to support them, but there is no conversation being had on this point. Simply labeling this as a "regression", a feature the API never explicitly supported, is wildly unfair.
    • The batch mode inputs are things I would be willing to implement if this PR is backed out of and if I am confident future work on the API will be consumed by the GUI. I was busy leading up to the GCC and BOSC - but trying to salvage the motivation to work on this component I've poured my life into for the last two years is my top Galaxy priority at this moment.
    • As for history refreshing - the "new" invocations endpoint is meant to be RESTful. I'm not sure how the API would trigger history refreshes without websockets - I'd say it is clearly the job of the client to grab the invocation ID and refresh the history as it is scheduled. Both the API tests and the bioblend test suite have examples of doing this.
@jxtx

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2016

@jmchilton Do you object to the compromise of having the workflow UI use a non-public non-API endpoint for the next release, and then working on changing the client / API / both to come up with a better solution in the next release? This lets us make some incremental progress while we reconcile the work you've done on creating an improved workflow invocation endpoint with the additional capabilities the workflow client UI needs (but, as you point out, were never part of the invocation API).

@jmchilton

This comment has been minimized.

Copy link
Member

commented Jul 18, 2016

@jxtx I'm still discouraged, but I did merge #2622 which does exactly this.

@martenson martenson referenced this pull request Sep 6, 2016

Closed

Reach parity of the old WF controller and the API #2899

6 of 8 tasks complete
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.