Proposed changes

[vsoch]

Hey @fgypas ! I just pushed a first draft for a spec, outlining the three main endpoints that you described. Along with getting feedback on the [spec.md] in the repo here https://github.com/panoptes-organization/monitor-schema/blob/main/spec.md, the following are some proposed changes, currently written into the spec.md (and all up for discussion!).

namespace

Akin to the OCI distribution spec, there is a common root (in their case, /v2/) that serves as the service endpoint, e.g., this would be the equivalent of the currently implemented /service-info/. The reason we want to do this is so that any client implementing the spec can be guaranteed of finding the endpoints at a server. My change here then would be using /m1/ as the root (indicating version 1 of the monitoring schema) and then replacing /service-info/ with /m1/. We could use something else other than m1, but I do think we want it to be simple, and unique enough so it's unlikely to conflict with a server's other endpoints. This endpoint also serves as an entrypoint to see if a server provides a monitor service, and optionally (but not enforced by the spec here) would be when authorization is handled.

/m1/

This is the proposed new service-info endpoint, and I'm proposing to add a "version" variable to the response body. That way, if there ultimately are differences in functionality for server versions, some client can act accordingly (although this would better be avoided).

flatten urls

instead of having something like /create_workflow/ I think we should have a spec that has a standard of /, so in this case it would be /workflow/create/ and the full namespaced url would be /m1/workflow/create/.

consistency of _ vs -

I noticed that there is switching back and forth between underscores and dashes (e.g., service-info and create_workflow) and I want to suggest that since we are pretty printing api endpoints, we use a dash (it just looks nicer), but only in the case there are any endpoints left with this structure.

LICENSE

I noticed in your license you put "Panoptes developers" and I think this is okay at a high level, but in practice it must be an individual or some entity (e.g., an LLC) for it to actually hold. I'm not a lawyer or similar, but for now I just put my name so it's an actual legal entity.

Questions for you!

In terms of authorization - how can a panoptes server be run and be guaranteed that it won't be discovered and sent malicious data? I might have missed it in the code, but I didn't see any sort of token (or similar) required. I'm thinking perhaps this might be described in the spec (that an authorization header is required) even if not required?

Just curious, what does panoptes mean? :)

And of course everything is up for discussion! I can also offer to do PRs to snakemake (or here) to make any necessary changes. I'll start my implementation of these endpoints when we are decided.

[fgypas]

Hey @fgypas ! I just pushed a first draft for a spec, outlining the three main endpoints that you described. Along with getting feedback on the [spec.md] in the repo here https://github.com/panoptes-organization/monitor-schema/blob/main/spec.md, the following are some proposed changes, currently written into the spec.md (and all up for discussion!).

Hi @vsoch Thanks a lot for this. It looks great and you were super fast. What I am not sure about and I have to check is whether the jobid is always needed. I think that the progress message is not always having a job id.

namespace

Akin to the OCI distribution spec, there is a common root (in their case, /v2/) that serves as the service endpoint, e.g., this would be the equivalent of the currently implemented /service-info/. The reason we want to do this is so that any client implementing the spec can be guaranteed of finding the endpoints at a server. My change here then would be using /m1/ as the root (indicating version 1 of the monitoring schema) and then replacing /service-info/ with /m1/. We could use something else other than m1, but I do think we want it to be simple, and unique enough so it's unlikely to conflict with a server's other endpoints. This endpoint also serves as an entrypoint to see if a server provides a monitor service, and optionally (but not enforced by the spec here) would be when authorization is handled.

/m1/

This is the proposed new service-info endpoint, and I'm proposing to add a "version" variable to the response body. That way, if there ultimately are differences in functionality for server versions, some client can act accordingly (although this would better be avoided).

I agree. Let's change it to /m1/.

flatten urls

instead of having something like /create_workflow/ I think we should have a spec that has a standard of /, so in this case it would be /workflow/create/ and the full namespaced url would be /m1/workflow/create/.

Yes, I think it would be better. Does this mean that both /workflow/create/ and /m1/workflow/create/ should be used, or only the latter one?

consistency of _ vs -

I noticed that there is switching back and forth between underscores and dashes (e.g., service-info and create_workflow) and I want to suggest that since we are pretty printing api endpoints, we use a dash (it just looks nicer), but only in the case there are any endpoints left with this structure.

I totally agree. Dash (-) is much better.

LICENSE

I noticed in your license you put "Panoptes developers" and I think this is okay at a high level, but in practice it must be an individual or some entity (e.g., an LLC) for it to actually hold. I'm not a lawyer or similar, but for now I just put my name so it's an actual legal entity.

I am also not sure about this. Maybe I should also change it on the main panoptes repo. Let's keep it like this for now, at least for this repo. Are you fine with the MIT license?

Questions for you!

In terms of authorization - how can a panoptes server be run and be guaranteed that it won't be discovered and sent malicious data? I might have missed it in the code, but I didn't see any sort of token (or similar) required. I'm thinking perhaps this might be described in the spec (that an authorization header is required) even if not required?

Well, we should add tokens, but we did not implement this yet. Is this what you are asking?

Just curious, what does panoptes mean? :)

It comes from this mythological many-eyed creature: https://en.wikipedia.org/wiki/Argus_Panoptes
In Greek panoptes means/is the one who sees everything.

And of course everything is up for discussion! I can also offer to do PRs to snakemake (or here) to make any necessary changes. I'll start my implementation of these endpoints when we are decided.

I think that we should update the code both in panoptes and snakemake. I think that the first thing we should do is to open an issue in snakemake describing what we want to do and implement the changes. Since we will do that, I think it makes sense to make some improvements. For example I think that the Logger that we are implementing should be a different class in snakemake (e.g. like https://github.com/snakemake/snakemake/blob/master/snakemake/logging.py#L84 or https://github.com/snakemake/snakemake/blob/master/snakemake/logging.py#L120). Can you maybe start with that? The changes that we made work fine, but I would feel good if we improve the codebase of snakemake.

[vsoch]

Thanks for the speedy feedback! Here are quick answers to your questions:

Yes, I think it would be better. Does this mean that both /workflow/create/ and /m1/workflow/create/ should be used, or only the latter one?

The client always has to be able to reliably find the endpoints, so if the /m1/ endpoint returned success to indicate that's the namespace for the monitor endpoints, then the correct endpoint would be /m1/workflow/create/.

I am also not sure about this. Maybe I should also change it on the main panoptes repo. Let's keep it like this for now, at least for this repo. Are you fine with the MIT license?

Yes definitely! I think I made sure to use the same license as I found for panoptes (double check, confirmed!)

Well, we should add tokens, but we did not implement this yet. Is this what you are asking?

Yes! I was thinking we should describe how to go about it, but it's not technically required to do (e.g., you could run it as you do now without tokens). For what I'm developing, the user should have an API token that is added to the request. We should probably ping Johannes for this since it would require a tweak to snakemake. But given that the panoptes (or similar) service is running on a shared resource, it would be important to protect the endpoints.

It comes from this mythological many-eyed creature: https://en.wikipedia.org/wiki/Argus_Panoptes
In Greek panoptes means/is the one who sees everything.

Oh I love that!

I think that we should update the code both in panoptes and snakemake. I think that the first thing we should do is to open an issue in snakemake describing what we want to do and implement the changes. Since we will do that, I think it makes sense to make some improvements. For example I think that the Logger that we are implementing should be a different class in snakemake (e.g. like https://github.com/snakemake/snakemake/blob/master/snakemake/logging.py#L84 or https://github.com/snakemake/snakemake/blob/master/snakemake/logging.py#L120). Can you maybe start with that? The changes that we made work fine, but I would feel good if we improve the codebase of snakemake.

I agree that Panoptes should be a Logger, proper. It would make sense to first pin the version of panoptes for snakemake (if it's not done already) and then develop the changes here, and then do the PR to snakemake with the updated version. I can help with both of those things - I'll open an issue on snakemake now describing what we'd like to do.

[johanneskoester]

Great work, I am eager to see the changes!