-
Notifications
You must be signed in to change notification settings - Fork 19
External service plugins #369
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
base: main
Are you sure you want to change the base?
Conversation
What does a plugin do vs a microservice? |
Hi @stefanv, It's the same thing! The idea here is just to allow folks to add custom micro services to their app without having to actually merge the service to the app's repo. A good example is using Fink broker in SkyPortal to get new. Doesn't make sense to add it to SkyPortal but one might one to add it to its own instance. This would allow that. Happy to change the name/terminology used to call this new feature. The reason why I called it plug-ins is that this could be extended to not just micro services but maybe front end and API one day in the future. |
Ah, in that case I wonder if we shouldn't just add the submodule functionality to the services? I'd also like to move port definitions and other configuration so that each service definition happens in one place. |
…were still registered
…the update if there are no modified files.
@stefanv btw we will stick with the "plugins installed via github" approach here and not "plugins installed from disk" that you mentioned, because well that is already entirely possible! in the baselayer config, one can just add paths to look for services. What this PR here does is just give users a way to grab them from github and git clone them into a specified path (already configurable in the config), and once that step is done the "plugin path" is simply appended to the paths where other services are located, so its quite simple and elegant, it does not edit much of the existing micro service discovery logic, but takes advantage of its existing path-driven modularity. TLDR: supporting additional microservices that are not pulled from some git repo is already natively supported in baselayer, so this PR just adds the git element to it (+ associated versioning check for those and other validation steps) before taking advantage of the existing micro service system. PS: What we could consider in a future PR is support for git sub modules, i.e. when we scan for microservices in an specified paths, if one of them is a non initialized github submodule make sure we initialize it and then treat it as any other microservices. That adds yet another way to add plugins, but in my opinion I prefer the system we've put in place in that PR because it encourages/forces the separate repo you're pulling from to specify versions and such. |
Oh and I committed some more fixes + some TODOs that @thomasculino is now addressing. |
…ed error handling
If you use the submodule path, the local path version also works just fine; you just need to do a checkout --recursive on your repo. |
Sure, though I think it's always good to do some validation, i.e warning, failing, or auto initializing submodules if baselayer starts and it's not done yet. |
And perhaps we could also enforce the same versioning (via toml) checks we do for plugins |
tools/setup_services.py
Outdated
|
||
def get_plugin_compatible_version(plugin_config: dict): | ||
# if the plugin specifies a "tool.<name>" section with | ||
# a version requirement, retrive it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# a version requirement, retrive it | |
# a version requirement, retrieve it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like a complete description; we return two things here, what are they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right, we'll add some proper docstrings for everything before requesting another review. This returns the name of the software in which we are installing the plugin and the version requirement we want to enforce on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
tools/setup_services.py
Outdated
""" | ||
Run a git command in the specified plugin path. | ||
|
||
Args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we use Google docstring format in the rest of skyportal? If not, make this consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nop, we'll change that. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
tools/setup_services.py
Outdated
plugin_name (str): Used in error logs | ||
|
||
Returns: | ||
stdout_lines (list of str): Output lines from stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does stdout get split into lines, but stderr is provided as a str?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @thomasculino needed to parse multiple lines for some commands, but we should standardize that. We'll talk about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
tools/setup_services.py
Outdated
|
||
for plugin_name, plugin_info in plugins.items(): | ||
if "url" not in plugin_info: | ||
log(f"Skipping plugin {plugin_name} because it has no URL") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log(f"Skipping plugin {plugin_name} because it has no URL") | |
log(f"Skipping plugin {plugin_name}: no URL found in config") |
Maybe provide some guidance here even, as to where and how user should specify config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed! Will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still a TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well examples are given in the config default, so maybe that's enough?
tools/setup_services.py
Outdated
version_tag = plugin_info.get("version") | ||
sha = plugin_info.get("sha") | ||
|
||
log(f"Cloning plugin {plugin_name}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing a deep clone, can we do a shallow clone of exactly the revision specified? If so, should be able to get rid of the three update_to_
functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we could, I never had to deal w/ shallow cloning before so might just need to look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much cheaper if you have a repo with long history, and the default way GH actions check out, e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
# TODO (in a future PR): loop over all services, check if they are a git submodule or not | ||
# if they are a submodule make sure they are initialized and updated | ||
# this should be discussed, it does not seem necessary as soon as we have the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think user can just take care of submodule updates, then plugin can be specified as file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand what you mean. You can just give a path in the config to add extra modules already. Are you saying we should take advantage of the plugin system instead here? But submodules have fixed branch+sha set so I think a lot of the logic does not apply for those, instead we might just want to init the submodule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advantages I see w/ using the plugin system for submodules is version validation + in general giving some standard place in the config for params that these submodules should read from. But we don't need to do any branch/sha/version management. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm thinking: the default services are essentially exactly the same as plugins, except they don't do version checking. So, a user can add submodules, check them out with their project, and augment the services path. We can update services to check skyportal version, if a pyproject.toml
is available. That way we have one unified pipeline for both, and plugins
just become a mechanism for checking out plugins from remote locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on our discussion, up to the user initialize their submodules and the existing baselayer config's services.paths will handle submodules anyways.
The pyproject.toml validation is now an optional and applied to all services, if the file is present.
Co-authored-by: Stefan van der Walt <[email protected]>
Co-authored-by: Stefan van der Walt <[email protected]>
… pyproject.toml) to all services, simplify some of the git clone/checkout logic, support either tomllib or tomli based on python version
Ready for review @Theodlz |
app/psa.py
Outdated
@@ -12,6 +12,7 @@ | |||
import warnings | |||
from datetime import datetime, timedelta | |||
|
|||
from baselayer.app.models import Base, DBSession, User |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pre-commit CI on baselayer keeps moving our imports! Doesn't look right to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some comments about the docs Thomas, nice job overall but a number of things that are part of the new plugin system (auto generated supervisor.conf, structure required for the plugins with pyproject.toml, optional config.yaml.defaults, ...) are omitted. These need to be in there.
Also not reflected in my individual suggestions and comments, but let's keep in mind that things like the optional config.yaml.defaults, automated supervisor.conf, and optional pyproject.toml are not external services specific at all!!! We now enforce that on all services, so I think it should move out of the external service-specific sections and be mentioned as micro-service registration feature(s) in general, and the external services stuff only explain what is specific to that system.
doc/usage.md
Outdated
External services can also be used to pull a custom microservice from a Github repository. | ||
|
||
They can also be added to the `config.yaml` file under the `services.external` key. Each service should have a unique name and a set of parameters that define what specific repository you want to pull. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
External services can also be used to pull a custom microservice from a Github repository. | |
They can also be added to the `config.yaml` file under the `services.external` key. Each service should have a unique name and a set of parameters that define what specific repository you want to pull. For example: | |
"External" micro-services can be used to extend the capabilities of any application relying on `baselayer`. | |
These are added to `services.external` section of the configuration, and need to provide: service name, GitHub repo URL, branch+sha or tagged version of the repo where the external service is hosted, along with optional service-specific parameters. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repeated can also
did not make much sense in this context, especially how it was repeated. That is my main comment, other changes aren't mandatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could do something simpler like:
"External" micro-services can be used to extend the capabilities of any application relying on `baselayer`.
These are added to `services.external` section of the configuration, providing the necessary information to register them alongside existing micro-services. For example:
doc/extending.md
Outdated
my_service: | ||
url: "https://github.com/my_service.git" | ||
branch: main | ||
sha: specific_commit_sha | ||
params: {} | ||
another_service: | ||
url: "https://github.com/another_service.git" | ||
version: v0.1.0 | ||
params: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an example of how to add params
(and to show it's optional), maybe it would make sense to have the first microservice not specify it, and the second one specify it but not empty (with some actual keys)?
doc/extending.md
Outdated
params: {} | ||
``` | ||
|
||
You must provide the `url` of the GitHub repository. To target a specific version of the service, you can optionally include both the `branch` and `sha`, or use the `version` field to refer to a particular release. Additional configuration options can be passed through the `params` dictionary, which the service may use during setup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a list of all supported keys, with (optional)
next to them where applicable, and a description would make this more readable? Just a recommendation / personal opinion, does not need to be done now. Let's perhaps see what Stefan thinks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can optionally include
doesn't seem correct. You HAVE to provide a branch+sha OR a version to add an external service, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional configuration options can be passed through the
params dictionary, which the service may use during setup.
I mean these are just keys the service may use, idk if it's necessarily at setup
time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe here is where it's good to mention:
- external services should provide a
main.py
, or if there entry point is different they have to provide asupervisord.conf
(which we otherwise auto generate to point to main.py - that they can provide a pyproject.toml file that contains the service's name (which is the same name that should be used in the config), and a
tool.X
section whereX
is the name of the application that is built on top of baselayer. And, that this section should include aversion
if specified, which is a version requirement, to make sure they only are registered and run with versions ofX
that they support. - that they can provide a
config.yaml.defaults
that contain a default configuration for the service, useful for external services that have default parameters (inparams:
), which also acts as an example of how to add the external service to an application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my overall comment about the docs in this file is that its not mentioning a number of things we spent time implementing, and that are required for a valid microservice to be built! The template repo and such are good examples but we need a place where the "how to build a microservice" and the "how they are registered" is detailed, otherwise no one will be able to use this intuitively!
doc/extending.md
Outdated
|
||
You must provide the `url` of the GitHub repository. To target a specific version of the service, you can optionally include both the `branch` and `sha`, or use the `version` field to refer to a particular release. Additional configuration options can be passed through the `params` dictionary, which the service may use during setup. | ||
|
||
The external service will then be initialized and registered in supervisor, alongside other services, provided it is correctly configured and compatible with the application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See for example here you talk about "compatibility" but it's never defined before, hence my previous comment
paths: | ||
- ./baselayer/services | ||
- ./services | ||
enabled: | ||
- cron | ||
disabled: '*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really needed to show these here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the way these work is detailed in the other file in greater detail, wondering if this one could just mention the external services' existence in a short paragraph and just refer to the other page. I'd say for now let's address my comments to clarify things and let's see what Stefan thinks. I feel like a bit of repetition isn't necessarily a bad thing actually, as long as it does not create more confusion.
doc/usage.md
Outdated
@@ -55,13 +56,35 @@ services: | |||
enabled: | |||
- cron | |||
disabled: '*' | |||
external: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessary to add external here in my opinion, it's an optional feature that isn't useful in the context of that example
paths: | ||
- ./baselayer/services | ||
- ./services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where it should be mentioned, but would be good to tell folks where external services are imported (i.e. in the last location specified in services.paths
)
Hey Stefan, I think this is ready for another round of review. You can test adding plugins by adding https://github.com/Theodlz/boom-skyportal-plugin, or Thomas' https://github.com/thomasculino/baselayer_external_service_template (the latter should be moved to the cesium organization imo). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the docs first, to ensure we are in agreement on higher level stuff, then will do a pass over the code 🙏
config.yaml.defaults
Outdated
# You can use disabled to disable specific services, or '*' to disable them all | ||
# Here you can add services that are not part of baselayer, but | ||
# come from another github repository. You need to specify URL, | ||
# branch+sha or version (tag), and optional . For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"and optional..." <- incomplete
# branch: <branch_name> | ||
# sha: <sha_of_commit> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
branch
and sha
could probably be same specified. Like, e.g., for pre-commit you would specify an action to run with https://github.com/some/action@branch
or https://github.com/some/action@abc123
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use both because we are using shallow clones. From my understanding, we would need to get rid of the shallow cloning if we want to use only one of branch
or sha
# sha: <sha_of_commit> | ||
# - my_service: | ||
# url: <url/to/git/repo> | ||
# version: <tagged_version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This too. Generic git term: ref
(reference
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
config.yaml.defaults
Outdated
@@ -156,3 +167,5 @@ security: | |||
# - interval: 5 | |||
# script: tools/5_minute.py | |||
# limit: ["01:00", "02:00"] | |||
|
|||
plugins: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why plugins, with external services already defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted, it was a mistake.
|
||
## Adding extrernal services | ||
|
||
If you want to add external services to your application, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I can't see higher up in that file, but we should check that the concept of a service is well defined. Also make very clear that external services are identical to services, they just live in remote repos.
|
||
#### Project Metadata and Compatibility | ||
|
||
External services may include a `pyproject.toml` file to provide metadata and compatibility information. This is especially important when multiple applications (or versions) are built on top of Baselayer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean: "especially important when multiple applications (or versions) are built on top of Baselayer"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Baselayer supports a variety of applications built on top of it. The pyproject.toml
file lets you specify compatibility for all supported applications, providing a centralized place to manage those configurations.
|
||
For example, an external service's `pyproject.toml` may look like: | ||
|
||
```toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Show a template example first, then discuss all possible keys. You can choose whether or not to include those keys in the template.
doc/extending.md
Outdated
{ name = "John Doe", email = "[email protected]" } | ||
] | ||
|
||
[tool.myapp] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to specify this as something like:
compatible-with = [...]
or similar, instead of requiring a key named after a package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a list of objects to specify the name and the compatible version.
[tool.compatibility]
compatible-with = [
{ name = "skyportal", version = ">=1.4.0" }
]
Co-authored-by: Stefan van der Walt <[email protected]>
Co-authored-by: Stefan van der Walt <[email protected]>
Co-authored-by: Stefan van der Walt <[email protected]>
Co-authored-by: Stefan van der Walt <[email protected]>
…into service_plugins
Co-authored-by: Stefan van der Walt <[email protected]>
…into service_plugins
@stefanv I believe this is ready for another round, Thomas made a lot of changes and answered your comments |
Just a quick experiment with plug-and-play microservices. Other instances of SkyPortal (such as Icare, used by GRANDMA) have some custom microservices. It's a little awkward that they have to maintain their special architecture to get just one microservice running.
This is just an idea of how we could allow one to add as many custom microservices as needed with just a few lines added to baselayer.
And, here's the demo repo that shows what a git repo for a custom microservice needs to look like:
https://github.com/Theodlz/skyportal-demo-plugin
Essentially, one can just add to the config's plugin section:
which will automatically download and update the plugin when calling
make run
to start the app.Definitely a WIP, but it works perfectly as is.