Repository navigation
Add RPC server support - #425
Conversation
43347bb to
e8da549
Compare
7587c29 to
7b70845
Compare
7b70845 to
8cb3ec2
Compare
* use rpc-server which runs invenio commands. this is a performance enhancement
* usefull for beta.dev versions
* The client used to spawn a full `uv run rpc-server send` subprocess per command, paying the interpreter and import cost the server exists to avoid, and that subprocess always exited 0, so failed commands were reported as successful steps. * `send_command()` now returns an `RPCCall` that speaks the JSON-lines protocol with stdlib socket+json and maps the response onto a `ProcessResponse`, so exit codes and stderr propagate and only the server needs the virtualenv. * The server is spawned lazily when the first command executes (not while the step list is being built), probed with a bounded retry loop instead of busy-spinning subprocess pings, and terminated at exit only if this process started it. * The socket lives at `<instance_path>/rpc.sock`, so RPC silently defers to plain commands until the instance path is known (first install).
* configparser `.get()` returns the raw string, so `use_rpc = false` in the `.invenio` file still enabled RPC. Read it with `getboolean` instead. * Drop the unused `rpc_port` option (the server no longer has a port) and update the README for the socket-based setup and the `use_rpc` flag.
* `CommandStep` now accepts an `RPCCall` as its cmd: it executes it over the socket, surfaces the buffered output (terminal or log file, like `run_interactive` does), and applies the same skippable-to-warning mapping. * This lets any step-based command switch to `send_command()` without changing the step plumbing. * The output handling is shared with the assets loop via a new `echo_output()` helper.
* All invenio commands in `services setup` (including the `--force` cleanup and demo steps) now go through `send_command()`, so with `use_rpc` enabled the whole sequence runs on one long-lived server instead of paying app startup per command. * Without `use_rpc` the behavior is unchanged. * The pybabel translation steps stay as plain subprocesses since they are not invenio CLI commands.
* `invenio_command()` (formerly `send_command`) now always returns a callable op: `RPCOp` when the RPC server is in use, `LocalOp` wrapping a plain subprocess otherwise. Both take the same arguments, so callers no longer branch on whether they got an argv list or a callable. * It is meant for short-lived, non-interactive invenio commands; everything else keeps using `run_command()`. * `RPCOp` sends the client's stdout/stderr (or the log file) along with the request using SCM_RIGHTS, and the server writes command output directly to them. Output shows up live in the terminal or log file like with a normal subprocess, webpack builds included, which also fixes the `--node-log-file` regression. * The captured-output mode is still available as `op(capture=True)` for callers that need the output programmatically, like `update_instance_path`.
* `RPCOp` no longer drops env. It is added to the Popen environment when the call is the one that spawns the server, which is the only place it can take effect: `INVENIO_*` variables and `FLASK_DEBUG` become app config at `create_app` time, so the auto-spawned server picks up e.g. pnpm's `INVENIO_WEBPACKEXT_NPM_PKG_CLS` from the assets steps. * Per-request env would not work, because the server caches the app and config-derived variables are only read at startup. * A server that is already running keeps the environment it was started with, so setting `WEBPACKEXT_NPM_PKG_CLS` in `invenio.cfg` remains the safe option for pnpm.
* The client no longer pings before sending: the single-threaded server queues the connection of a second client until the current command finishes, whereas the old 2s ping timeout made a busy server look absent and triggered a doomed second spawn. * Only a refused or missing socket (nothing listening) now causes a spawn, probed with a cheap connect check that also recognizes a busy server. * If the spawned server dies during startup, the client rechecks the socket before reporting failure, so losing a spawn race against another invenio-cli whose server won the socket is treated as success.
* `start_containers()` captured the compose output through `run_cmd` and `ensure_containers_running()` discarded the response, so `docker compose up -d` showed nothing while pulling images for minutes and a compose failure only surfaced as a healthcheck timeout much later. * The command now runs interactively (compose renders its live progress) and a non-zero exit fails the step immediately.
* The rebase that introduced the `app_rdm` version pin changed `rdm_version()` to require `cli_config`, but the call sites added upstream in the meantime (`run_jobs_scheduler`, containers setup, `check-requirements`) still called it bare and raised TypeError. * The worst case was `invenio-cli run`: the crash happened after the web server and celery worker were already spawned, so invenio-cli died and orphaned both. That is why celery kept running after Ctrl+C. * `cli_config` is now optional (`check-requirements` has no project context at all), the pin takes precedence when one is passed, and the callers that do have a config pass it.
* Nothing sends a protocol ping anymore: liveness is a connect-only probe (a busy server still accepts connections) and real requests surface their own errors. * Remove ping() and the read-timeout plumbing from the client, and make the test servers tolerate the empty read of a connect probe.
* 'invenio-cli install' from a clean environment never used the RPC server: the package manager resolved the socket path once at construction, before update_instance_path had discovered the instance directory, so the whole install ran as subprocesses. * The manager now receives a callable returning the socket path (get_rpc_socket_path, passed create_app-style) and builds the RPC client lazily on the first invenio_command() call that sees one, so the assets steps right after the instance path is written already go through the server.
8cb3ec2 to
c41294d
Compare
fenekku
left a comment
There was a problem hiding this comment.
Couple of defaults to change and some sections we can just remove. Then it's all good. Approving pending those changes since I will be out the next couple of days and don't want to block.
| ============== | ||
|
|
||
| It is possible to choose between two python package managers: `pipenv` and `uv`. | ||
| `pipenv` is the default one. To customize the python package manager it is |
There was a problem hiding this comment.
uv should be the default one now and the rest of the text adapted.
There was a problem hiding this comment.
I guess there isn't a default as much as a forced choice now, so we can remove any reference to "defaults" now. Something like:
It is possible to choose between two python package managers: `uv` or `pipenv`. To customize...| file. `VALUE` is `uv` or `pipenv`. | ||
|
|
||
| It is possible to choose between two javascript package managers: `npm` and | ||
| `pnpm`. `npm` is the default one. To customize the python package manager it is |
There was a problem hiding this comment.
pnpm is the default one now
There was a problem hiding this comment.
oh I see this was reverted: https://github.com/inveniosoftware/invenio-cli/pull/435/changes ... but PNPM is the value written out to the config https://github.com/inveniosoftware/invenio-cli/blob/master/invenio_cli/helpers/cli_config.py#L216 .
So something like:
It is possible to choose between two Javascript package managers: `npm` or
`pnpm`. `npm` is chosen if none are specified. | file. `VALUE` is `npm` or `pnpm`. | ||
|
|
||
| It is possible to choose between two assets builders: `webpack` and `rspack`. | ||
| `webpack` is the default one. To use `rspack` add `WEBPACKEXT_PROJECT = |
There was a problem hiding this comment.
rspack has been set as default in the invenio.cfg cookiecutter so that's also the default (perhaps a little "less so" than the other ones since they are default in invenio-cli code and not just config).
| The javascript package manager uses a lock file like the python package manager. | ||
| This file `pnpm-lock.yaml` for `pnpm` and `packages-lock.json` for `npm` will be | ||
| symlinked to the `var/instance/assets/` directory. | ||
|
|
There was a problem hiding this comment.
I am not sure how this section relates to the "Customizations" header 🤔. I would just remove that paragraph.
| Hints | ||
| ===== | ||
|
|
||
| `uv` | ||
|
|
||
| The development with `uv` is a little bit different than with `pipenv`. If there | ||
| is a `uv.lock` file packages have to be updated manually or by removing the | ||
| `uv.lock` file. The absence of the `uv.lock` file triggeres a new dependency | ||
| resolving call which takes into account of new released packages. It would also | ||
| be possible to use the `uv sync --upgrade` feature of `uv` but this installs | ||
| also beta versions of packages which is not recommended. It may sound strange to | ||
| remove the `uv.lock` file, but `uv` is that fast that deleting the `.venv` | ||
| directory and the `uv.lock` file is the easiest and fastest and safest way to | ||
| upgrade the packages. | ||
|
|
||
| UV uses a local `.venv` directory. This will be created automatically, if not | ||
| existing in the same directory as the `pyproject.toml` file. | ||
|
|
||
| UV uses a `pyproject.toml` file. | ||
|
|
||
| `rpc-server` | ||
|
|
||
| To use `pnpm` with the `rpc-server` it is necessary to add | ||
| `WEBPACKEXT_NPM_PKG_CLS = "pynpm:PNPMPackage"` to the `invenio.cfg` file. |
There was a problem hiding this comment.
I would actually remove that part too. We can simply provide a link to https://docs.astral.sh/uv/` in the section above and not bother with "hints" (too bogged down).
* remove any misleading text
1d5ed43 to
33c768b
Compare
Depends on inveniosoftware/invenio-app#113
enhancement
refactor(rpc): talk to the RPC server directly over its socket
uv run rpc-server sendsubprocess per command, paying the interpreter and import cost the server exists to avoid, and that subprocess always exited 0, so failed commands were reported as successful steps.send_command()now returns anRPCCallthat speaks the JSON-lines protocol with stdlib socket+json and maps the response onto aProcessResponse, so exit codes and stderr propagate and only the server needs the virtualenv.<instance_path>/rpc.sock, so RPC silently defers to plain commands until the instance path is known (first install).fix(rpc): parse use_rpc as a boolean and document the socket setup
.get()returns the raw string, souse_rpc = falsein the.inveniofile still enabled RPC. Read it withgetbooleaninstead.rpc_portoption (the server no longer has a port) and update the README for the socket-based setup and theuse_rpcflag.feat(rpc): let CommandStep execute RPC calls
CommandStepnow accepts anRPCCallas its cmd: it executes it over the socket, surfaces the buffered output (terminal or log file, likerun_interactivedoes), and applies the same skippable-to-warning mapping.send_command()without changing the step plumbing.echo_output()helper.feat(rpc): route services setup through the RPC server
services setup(including the--forcecleanup and demo steps) now go throughsend_command(), so withuse_rpcenabled the whole sequence runs on one long-lived server instead of paying app startup per command.use_rpcthe behavior is unchanged.feat(rpc): stream output via fd passing and unify command ops
invenio_command()(formerlysend_command) now always returns a callable op:RPCOpwhen the RPC server is in use,LocalOpwrapping a plain subprocess otherwise. Both take the same arguments, so callers no longer branch on whether they got an argv list or a callable.run_command().RPCOpsends the client's stdout/stderr (or the log file) along with the request using SCM_RIGHTS, and the server writes command output directly to them. Output shows up live in the terminal or log file like with a normal subprocess, webpack builds included, which also fixes the--node-log-fileregression.op(capture=True)for callers that need the output programmatically, likeupdate_instance_path.feat(rpc): apply env when spawning the server
RPCOpno longer drops env. It is added to the Popen environment when the call is the one that spawns the server, which is the only place it can take effect:INVENIO_*variables andFLASK_DEBUGbecome app config atcreate_apptime, so the auto-spawned server picks up e.g. pnpm'sINVENIO_WEBPACKEXT_NPM_PKG_CLSfrom the assets steps.WEBPACKEXT_NPM_PKG_CLSininvenio.cfgremains the safe option for pnpm.fix(rpc): connect directly instead of pinging before each call
fix(services): stream docker compose output and fail on errors
start_containers()captured the compose output throughrun_cmdandensure_containers_running()discarded the response, sodocker compose up -dshowed nothing while pulling images for minutes and a compose failure only surfaced as a healthcheck timeout much later.fix(versions): make cli_config optional in rdm_version
app_rdmversion pin changedrdm_version()to requirecli_config, but the call sites added upstream in the meantime (run_jobs_scheduler, containers setup,check-requirements) still called it bare and raised TypeError.invenio-cli run: the crash happened after the web server and celery worker were already spawned, so invenio-cli died and orphaned both. That is why celery kept running after Ctrl+C.cli_configis now optional (check-requirementshas no project context at all), the pin takes precedence when one is passed, and the callers that do have a config pass it.