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

adding extra views to spec #7

Merged
merged 4 commits into from
May 27, 2021
Merged

adding extra views to spec #7

merged 4 commits into from
May 27, 2021

Conversation

vsoch
Copy link
Collaborator

@vsoch vsoch commented Dec 15, 2020

Here is an update to the schema to add the remaining views! I opened some issues with questions, and we can further discuss here.

Timestamps

I chose a fairly straight forward standard, and I'm wondering if we can decide to use it?

Update vs rename

It looks like rename is a scoped update (see issue #6). To start I can implement the views separately, but I'm wondering if we should have a single update API endpoint that can handle some subset of parameters?

507 Response

What does this mean (in human friendly terms that I can understand, haha)

workflows vs jobs

I noticed that for a list of workflows or jobs, we return a response with an index like:

{"jobs": []}
{"workflows": []}

But then for a single workflow we just return

{"workflow": []}

but for a single job we return

{"jobs": []}

Should these be consistently plural / single, depending on the endpoint? And for a single entity, why should we return it nested if the count is expected to be 1?

And overall, take a look at the updates, required / optional fields, and let me know what you think! I changed the update endpoint also to just be a POST to the same /m1/workflow endpoint. What I can add at a later time (after this PR) is a nice table that quickly shows all endpoints together to get a sense of the whole picture.

Signed-off-by: vsoch vsochat@stanford.edu

Signed-off-by: vsoch <vsochat@stanford.edu>
@fgypas
Copy link
Member

fgypas commented Dec 16, 2020

@vsoch One thing I realized that is missing is a Cancel workflow option and consequently a cancelled status.

@vsoch
Copy link
Collaborator Author

vsoch commented Dec 16, 2020

Ah good call! I'll add that to the PR now.

Signed-off-by: vsoch <vsochat@stanford.edu>
Copy link
Member

@fgypas fgypas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just found one minor thing. The rest is fine. The only thing I am still a bit confused about (and I need a bit more time to think) is what's the best option for the workflow id (I will comment on the issues).

spec.md Outdated Show resolved Hide resolved
Signed-off-by: vsoch <vsochat@stanford.edu>
@vsoch
Copy link
Collaborator Author

vsoch commented Dec 16, 2020

@fgypas let me know when this is good to merge, or ping me on other issues to discuss points further!

@fgypas fgypas self-requested a review December 16, 2020 22:12
Signed-off-by: vsoch <vsochat@stanford.edu>
@fgypas
Copy link
Member

fgypas commented Dec 20, 2020

Hi @vsoch
Another thing I was thinking about are the logs of the jobs.
What I mean is that it is useful to display the log messages in the web-interface. Especially for cases when you get an error, it is convenient to show the error log.
What do you think? Should we include it in the specs as well?

@vsoch
Copy link
Collaborator Author

vsoch commented Dec 20, 2020

There is a log attribute in the update message, wouldn’t that be enough for the spec? It’s out of scope I think to say how the interface should be designed - there could for example be a client that conforms to the spec but doesn’t even have an interface.

@vsoch
Copy link
Collaborator Author

vsoch commented Dec 20, 2020

Also I pinged Johannes to ask for feedback on needing to add an ID last week - haven't heard anything yet! I do think we'd want to get his blessing about what we are allowed to provide to snakemake before moving forward doing it.

@fgypas
Copy link
Member

fgypas commented Dec 21, 2020

Yes, let's wait for his response, before we add new changes. It's holiday season anyway and most things move slower than usual.

@vsoch
Copy link
Collaborator Author

vsoch commented Dec 21, 2020

hey @fgypas ! If you didn't see it, Johannes responded with feedback about ids. #5 (comment) Let me know if there is anything else we should add here before we can merge and then work on panoptes.

@fgypas
Copy link
Member

fgypas commented Dec 23, 2020

Hi @vsoch

Thanks. Yes, I saw the message and I just sent a new question to Johannes (maybe you already know?).

I also asked @gkostoulas who has more experience than me on APIs to have a look at the pull request.

An additional thing is that maybe it would be a good idea to line-up our API to the API of GA4GH. I am not sure if you are aware of that, but there are two APIs: (WES and TES) that kind of fit our needs. Please let me know what you think.

@vsoch
Copy link
Collaborator Author

vsoch commented Dec 23, 2020

I think that's a fantastic idea! I'd even say drop the monitor schema we have here if this is a well known, accepted API that can be used here. In the case that we want to provide more detail or extend it, then possibly we would need to keep the information here.

Anyway - let me know what you would like to do, I'm definitely around over the holiday!

@fgypas
Copy link
Member

fgypas commented Dec 24, 2020

Hi @vsoch

For running pipelines I think WES is the way to go. So for snakeface I would use it. For TES, I think that we could partially support it, as we do not submit individual tasks. So the get endpoints we could implement, but not the post. I think that we will have to postpone this for a while until we reach a consensus (more feedback).

Meanwhile, if you have time and you want, you can have a look at the following issue:
panoptes-organization/panoptes#2

This is a feature that we need to add to snakemake in order to have some authentication/authorization (token support) in snakeface and panoptes. I think we need to have this as an environmental variable. Snakemake can then use the token generated to communicate with the servers. What do you think? I will try to keep an eye on the issues during the holiday.

@vsoch
Copy link
Collaborator Author

vsoch commented Dec 24, 2020

They look almost the same, except one is relevant to a task, the other a workflow. What I don't obviously see are endpoints for updating a workflow, e.g., you can create and then list but that's otherwise it. It also doesn't appear that you have any power to namespace the endpoints. :/

@vsoch
Copy link
Collaborator Author

vsoch commented Dec 26, 2020

Meanwhile, if you have time and you want, you can have a look at the following issue:
panoptes-organization/panoptes#2

This is a feature that we need to add to snakemake in order to have some authentication/authorization (token support) in snakeface and panoptes. I think we need to have this as an environmental variable. Snakemake can then use the token generated to communicate with the servers. What do you think? I will try to keep an eye on the issues during the holiday.

This is definitely needed! I'm starting to get close to implementing the current snakemake endpoints for snakeface, and when I do that I can more easily test a PR that uses authentication. Looking at the very general suggestion to provide a bearer token, I'm thinking that what we might want to do is to generate a token that is specific to a workflow run, and then provide that, and either it could expire when that workflow run finishes, after some amount of time, or never. The server would be generating and receiving the token, so it's a simple strategy to not let some random person update it. If the user from the command line would want to run a workflow again, then they would need to export it in the environment. What do you think?

@fgypas
Copy link
Member

fgypas commented May 27, 2021

Hi @vsoch Long time no see. Sorry, for not getting back to you earlier. I have more time now to work on the open issues. How would you like to proceed? Do you want to use the schema you proposed or something else? I am fine with whatever you prefer.

@vsoch
Copy link
Collaborator Author

vsoch commented May 27, 2021

This schema would work for me! I believe I already implemented the endpoints into snakeface. I haven't had anyone express interest in the tool so I haven't worked on it since April, but if anyone would express interest and give feedback I could jump back to it pretty easily!

@fgypas
Copy link
Member

fgypas commented May 27, 2021

Ok, let's use this one then. Actually, I have a use case where snakeface would fit perfectly.

@fgypas fgypas merged commit 42c550d into main May 27, 2021
@vsoch vsoch deleted the add/extra-views branch May 27, 2021 13:19
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.

None yet

2 participants