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

Rename misleading "workflows" endpoints as "workflow-runs" #32

Merged
merged 4 commits into from
Jul 2, 2018
Merged

Rename misleading "workflows" endpoints as "workflow-runs" #32

merged 4 commits into from
Jul 2, 2018

Conversation

jaeddy
Copy link
Member

@jaeddy jaeddy commented Jun 6, 2018

This resurfaces @junjun-zhang's PR, which used the term job to denote workflows that have been submitted to WES. Based on some discussions in Toronto, I changed "job" to "run" throughout the spec. I'm not sure if people have a strong preference between the use of Run vs. WorkflowRun for some endpoints.

To view the (extended) discussion around the original PR, refer to #26

note: depending on what gets approved first, I can merge these changes with #30 and/or #31 from @tetron.

@geoffjentry
Copy link
Contributor

re run vs workflowRun (I'm not militant on things like camel, snake, etc) I'd prefer the latter.

@jaeddy
Copy link
Member Author

jaeddy commented Jun 6, 2018

Agreed. Fixed.

@briandoconnor
Copy link
Contributor

OK, looks good to me although I think we need to update the API version number before the next release.

@briandoconnor briandoconnor merged commit 67729e4 into ga4gh:develop Jul 2, 2018
@dglazer
Copy link
Member

dglazer commented Jul 2, 2018

Per today's call, we're going to revisit this in a smaller group and report back.

@dglazer
Copy link
Member

dglazer commented Jul 2, 2018

@jaeddy @geoffjentry @mckinsel @tetron -- per discussion at and after today's Cloud WS call, @briandoconnor and I think that this PR was merged prematurely, and that we should use the approval process we agreed on in Toronto to finalize a naming decision. That means we need votes from the four of you as the hands-on implementors, plus Brian and me as co-chairs.

Please add your vote here on which of the choices below you prefer. I suggest the following votes (borrowed from the old task teams, who in turn borrowed it from the IETF):
+1 means "I approve this option"
+0 means "I'm fine with this option"
-0 means "I don't like this option, but won't block it"
-1 means "I veto this option, until someone convinces me otherwise"

Here are the two choices that seem to have the most traction. (Write-in votes are also okay, but only if you feel strongly they're better than these two, so we can wrap this up.)

Option A ("run")
POST /runs, aka RunWorkflow() -- takes a static thing and returns a running thing
DELETE /runs/{run_id}, aka CancelRun() -- cancels a running thing
GET /runs, aka ListRuns() -- returns a list of running things
GET /runs/{run_id}/status, aka GetRunStatus() -- returns info about a running thing
GET /runs/{run_id}, aka GetRunLog() -- returns more info about a running thing

Option B ("workflow-run"), as in the current version of the PR:
POST /workflow-runs, aka RunWorkflow() -- takes a static thing and returns a running thing
DELETE /workflow-runs/{run_id}, aka CancelWorkflowRun() -- cancels a running thing
GET /workflow-runs, aka ListWorkflowRuns() -- returns a list of running things
GET /workflow-runs/{run_id}/status, aka GetWorkflowRunStatus() -- returns info about a running thing
GET /workflow-runs/{run_id}, aka GetWorkflowRunLog() -- returns more info about a running thing

@dglazer
Copy link
Member

dglazer commented Jul 2, 2018

My personal vote:
+1 for Option A ("run")
-0 for Option B ("/workflow-run") -- mostly on aesthetic reasons; I think it's clunky
I'd also be +1 for "job", but I think others have concerns with it

@pgrosu
Copy link

pgrosu commented Jul 2, 2018

@dglazer Could you please let me know where the smaller group discussion will take place?

Thanks,
Paul

@jaeddy
Copy link
Member Author

jaeddy commented Jul 2, 2018

+1 for Option A
-0 for Option B
+0 for '/s/run/job/' Option A

@geoffjentry
Copy link
Contributor

+0 to both options A & B.

-1 to the use of the word job anywhere

@dglazer
Copy link
Member

dglazer commented Jul 2, 2018

@pgrosu -- smaller group discussion on process already happened -- outcome is my comment from a couple of hours ago

@tetron
Copy link
Contributor

tetron commented Jul 3, 2018

At first I was leaning towards option B (workflow-runs) because it contains the term "workflow" so it is less ambiguous, however it is already under the "ga4gh/wes" namespace so "workflow" is already implied:

/ga4gh/wes/v1/runs

vs

/ga4gh/wes/v1/workflow-runs

So +1 on option A and +0 on option B.

@jaeddy
Copy link
Member Author

jaeddy commented Jul 3, 2018

ha @tetron, I said pretty much the same thing in an email thread about this topic a couple weeks ago:

I'm guessing some might argue that run is too ambiguous (i.e., what am I "running"?), but I think sufficient context is provided by the fact that it's the "workflow execution service" API. In other words, the "WES" namespace should make it clear enough that client operations like wes.get_runs() are providing information about "workflow runs".

👍

@mckinsel
Copy link

mckinsel commented Jul 5, 2018

Hey, I'm +0 on A and B.

@dglazer
Copy link
Member

dglazer commented Jul 5, 2018

Thanks all -- here's a summary (Brian voted off-thread):

A: 4 +1, 2 +0 (dglazer +1, jaeddy +1, geoffjentry +0, tetron +1, broconnor +1, mckinsel +0) <== "run"
B: 4 +0, 2 -0 (dglazer -0, jaeddy -0, geoffjentry +0, tetron +0, broconnor +0, mckinsel +0) <== "workflow-run"

Looks like A is the favorite of everyone who has a preference. @jaeddy -- are you up for updating the code one last time, and then we can put this to bed? Thanks.

@jaeddy
Copy link
Member Author

jaeddy commented Jul 5, 2018

Thanks, @dglazer! I'll update the code today or tomorrow.

@pgrosu
Copy link

pgrosu commented Jul 7, 2018

Thanks @dglazer. So just thinking about this, we currently operate on the following definitions:

wes-defs

I'm also okay with calling it runs (+1), as it the "workflow execution service''. The only thing is we might encounter complex workflows that could require different levels of granularity in terms of monitoring, then we don't know if is a tool or workflow run, which makes it surjective. If we call it jobs, then we know it must be workflows, and then we need one called tasks for tools, but that could get confusing. Once we create snapshots for exploring other variations on different instance runs, things might become harder to integrate and distribute. Anyway, let's see how far this takes us - less might be more :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants