panoptes-organization / monitor-schema

MIT License
3 stars 4 forks source link

adding extra views to spec #7

Closed vsoch closed 3 years ago

vsoch commented 3 years ago

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

fgypas commented 3 years ago

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

vsoch commented 3 years ago

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

vsoch commented 3 years ago

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

fgypas commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

hey @fgypas ! If you didn't see it, Johannes responded with feedback about ids. https://github.com/panoptes-organization/monitor-schema/issues/5#issuecomment-748905234 Let me know if there is anything else we should add here before we can merge and then work on panoptes.

fgypas commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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: https://github.com/panoptes-organization/panoptes/issues/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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