Add `system caches` `list` and `delete` subcommands to manage runtime caches by toph-allen · Pull Request #384 · posit-dev/rsconnect-python

toph-allen · 2023-03-24T17:48:36Z

Intent

Add new subcommands to call Connect's forthcoming runtime cache management API.

Type of Change

Bug Fix
New Feature
Breaking Change

Approach

Note: CI is failing because of one of the Connect-in-Docker integration tests I wrote, where I try to use Python's system() to run commands in the Docker container. See below for more details. I'll take another shot at it on Monday.

There are three main functional components:

Low-level functions in RSConnectClient that call the endpoints and return the response.
Higher-level functions in RSConnectExecutor.
Command-line definitions.

The list command is very simple: it either serializes the JSON response from the server, or prints the error.

The delete command is more complex, because it has a dry-run case, which doesn't result in a server-side task, and a non-dry-run case, which does result in the creation of a task. The logic to handle this is in RSConnectExecutor.delete_runtime_cache().

Dry-run invocations of delete contain "task_id": null. In this case, we print "Dry run finished", as the server doesn't log anything. If the command is called with -v, the result will also be printed. In the dry run case, if the deletion wouldn't happen for any reason (e.g. the cache doesn't exist) the endpoint will return an error.
Wet-run invocations of delete will have a non-null task_id. The endpoint will call client.wait_for_task() and log server messages from that task, which include a start and a success message. The task status object is silently returned at the end, but it'll be printed with -v.

Automated Tests

The API is tested on two levels:

Unit tests in test_api.py
- These use httpretty to mock responses to confirm that the endpoints function as desired when given the right inputs.
Integration tests building on the approach used by the vetiver tests, in -test_main_system_caches.py
- These spin up a Connect server and use click.CliRunner() to test the end-to-end functionality.

QA Notes

We'll manually test that we can enumerate and delete caches.

Spin up a Connect server.
Create a fake cache directory, e.g. mkdir pip/1.2.3 within the python-environments directory.
Use rsconnect system caches list to list the runtime caches.
Use rsconnect system caches delete --language Python --version 1.2.3 --image-name Local to delete the cache directory.

Checklist

I have updated CHANGELOG.md to cover notable changes.
I have updated all related GitHub issues to reflect their current state.

I have not updated CHANGELOG.md yet. Will do so before this merges.

Co-authored-by: Bincheng Wu <bcwu@users.noreply.github.com>

tdstein

Tests that depend on docker are isolated from unit tests within the project structure. In my opinion, the integration tests should be moved to another directory to separate them from the unit tests. Then a command should be added to the Makefile, which invokes them independently.

That way, I should be able to run make integration-tests from my machine and get the same results as running the same command in GitHub Actions.

Also, this is the first time I've seen setup and teardown Docker commands invoked as part of the unit test lifecycle. I have always utilized shell scripts to achieve this. Are there any pros or cons to running the Docker commands via the Python os package?

tdstein · 2023-03-27T16:25:30Z

Thank you! This refactor was needed!

tdstein · 2023-03-27T16:28:33Z

+          python -m unittest tests.test_main_system_caches
          pytest -m 'vetiver'


Suggested change

python -m unittest tests.test_main_system_caches

pytest -m 'vetiver'

pytest tests/test_main_system_caches.py

pytest -m 'vetiver'

This should also work.

toph-allen · 2023-03-27T20:34:50Z

Tests that depend on docker are isolated from unit tests within the project structure. In my opinion, the integration tests should be moved to another directory to separate them from the unit tests. Then a command should be added to the Makefile, which invokes them independently.

That way, I should be able to run make integration-tests from my machine and get the same results as running the same command in GitHub Actions.

Also, this is the first time I've seen setup and teardown Docker commands invoked as part of the unit test lifecycle. I have always utilized shell scripts to achieve this. Are there any pros or cons to running the Docker commands via the Python os package?

Yeah, having these tests as unit tests that are just skipped if the docker service isn't available seems really hacky. I agree that there should be a single make target to run them.

There's already an integration-tests directory, which — I'm not sure what it's used for. I was initially thinking about renaming the vetiver-tests directory to integration-tests till I saw there was already a directory there, because I'm trying to contain the scope of this PR to getting this cache deletion story done and not trying to make infrastructure changes to rsconnect-python. Probably makes sense to put it into a follow-up PR? @bcwu thoughts?

bcwu · 2023-03-27T20:58:47Z

Tests that depend on docker are isolated from unit tests within the project structure. In my opinion, the integration tests should be moved to another directory to separate them from the unit tests. Then a command should be added to the Makefile, which invokes them independently.
That way, I should be able to run make integration-tests from my machine and get the same results as running the same command in GitHub Actions.
Also, this is the first time I've seen setup and teardown Docker commands invoked as part of the unit test lifecycle. I have always utilized shell scripts to achieve this. Are there any pros or cons to running the Docker commands via the Python os package?

Yeah, having these tests as unit tests that are just skipped if the docker service isn't available seems really hacky. I agree that there should be a single make target to run them.

There's already an integration-tests directory, which — I'm not sure what it's used for. I was initially thinking about renaming the vetiver-tests directory to integration-tests till I saw there was already a directory there, because I'm trying to contain the scope of this PR to getting this cache deletion story done and not trying to make infrastructure changes to rsconnect-python. Probably makes sense to put it into a follow-up PR? @bcwu thoughts?

A separate PR makes sense. For now, feel free to make as much (or as little) change you feel is needed to get the feature ready. That is to say, if we can get the tests pass CI, feel free to leave them in the PR.

toph-allen · 2023-03-27T21:07:41Z

@bcwu @tdstein Right now the Vetiver integration test workflow uses the local make dev target that's used for running locally. This uses the -T flag, which makes it run as a TTY, which is causing the integration test failures atm.

The dev target is currently split across a few sub targets. Roughly excerpted:

RSC_API_KEYS=vetiver-testing/rsconnect_api_keys.json

...

dev: vetiver-testing/rsconnect_api_keys.json

dev-start:
	docker-compose up -d
	docker-compose exec -T rsconnect bash < vetiver-testing/setup-rsconnect/add-users.sh
	# curl fails with error 52 without a short sleep....
	sleep 5
	curl -s --retry 10 --retry-connrefused http://localhost:3939

...

$(RSC_API_KEYS): dev-start
	python vetiver-testing/setup-rsconnect/dump_api_keys.py $@

But the GitHub Actions .yml also calls docker-compose up --build -d just before it calls make dev.

I've honestly hardly ever used make — Connect switched to just right when I joined the team — so it's not immediately obvious what the simplest way to make the -T change in CI is without breaking the local dev setup or making the Makefile overly complex. I'll look into that now.

aronatkins · 2023-03-29T16:02:30Z

+## Unreleased
+
+### Added
+- Added `system caches list` and `system caches delete` commands which allow administrators to enumerate and delete R and Python runtime caches from Connect servers.


We will want to indicate the minimum Connect release having this functionality.

aronatkins · 2023-03-29T16:05:59Z

+    {
+      "language": "Python",
+      "version": "3.8.12",
+      "image_name": "teapot"


Same for Python - maybe use one of the Python images to reference a real one? https://hub.docker.com/_/python/

aronatkins · 2023-03-29T16:07:54Z

+You should run this command for each cache you wish to delete.
+
+To determine whether your a deletion request would fail without risking
+deletion, you can use the dry run option (`-d, --dry-run`) to surface any errors


Suggest the dry-run as an integral part of the workflow, not an afterthought.

dry-run (with example)

delete (with example)

- language is case insensitive - image-name is optional, default “Local” - docs add more detail about parameters

Title-casing the language doesn't harm functionality, but it might confuse my future self.

toph-allen · 2023-04-11T22:24:49Z

Just noting here that @aronatkins, @ChaitaC and I verified this on Thursday, March 30th in our Refinement meeting. I failed to record that when it happened.

This is holding till the required Connect functionality releases.

toph-allen added 5 commits March 17, 2023 16:56

First implementation of list and delete endpoints

afaec14

First pass at API unit tests

cf2adba

update docker-compose to use proper image

0e5c3b0

add integration tests; run make fmt

55c38e5

run tests in CI; skip if rsconnect service missing

214510d

toph-allen changed the title ~~Toph system caches runtime~~ Add system caches list and delete subcommands to manage runtime caches Mar 24, 2023

Simplify logging.

53bf085

toph-allen force-pushed the toph-system-caches-runtime branch from d6c626a to 53bf085 Compare March 24, 2023 18:05

toph-allen requested review from aronatkins and mmarchetti March 24, 2023 18:14

toph-allen marked this pull request as ready for review March 24, 2023 18:16

CI fixes

a3e264d

toph-allen force-pushed the toph-system-caches-runtime branch from ca4828b to a3e264d Compare March 24, 2023 18:17

reorder tests so they work right

76c7ddb

bcwu reviewed Mar 24, 2023

View reviewed changes

Comment thread rsconnect/main.py Outdated

bcwu reviewed Mar 24, 2023

View reviewed changes

Comment thread rsconnect/main.py Outdated

bcwu reviewed Mar 24, 2023

View reviewed changes

Comment thread rsconnect/main.py Outdated

toph-allen and others added 4 commits March 24, 2023 16:00

Update rsconnect/main.py

5315459

Co-authored-by: Bincheng Wu <bcwu@users.noreply.github.com>

Update rsconnect/main.py

f4a0548

Co-authored-by: Bincheng Wu <bcwu@users.noreply.github.com>

Insert @server_args where appropriate

d1ab101

Merge branch 'master' into toph-system-caches-runtime

3612171

toph-allen requested a review from bcwu March 24, 2023 21:48

aronatkins approved these changes Mar 27, 2023

View reviewed changes

bcwu reviewed Mar 27, 2023

View reviewed changes

Comment thread rsconnect/api.py Outdated

bcwu reviewed Mar 27, 2023

View reviewed changes

Comment thread rsconnect/api.py

tdstein reviewed Mar 27, 2023

View reviewed changes

Respond to review

624e875

attempt to fix failing vetiver test

c27fbf8

toph-allen force-pushed the toph-system-caches-runtime branch from c7f1037 to c27fbf8 Compare March 28, 2023 15:29

toph-allen requested review from bcwu and tdstein March 28, 2023 15:41

Update changelog.

0821c56

toph-allen requested a review from aronatkins March 29, 2023 15:55

Add new commands to readme

d29db3b

toph-allen force-pushed the toph-system-caches-runtime branch from 19fc913 to d29db3b Compare March 29, 2023 15:56

aronatkins approved these changes Mar 29, 2023

View reviewed changes

toph-allen added 3 commits March 29, 2023 12:34

Respond to review comments, mess with David's shell blocks

f05bfaa

play with admonitions

ef21777

play with admonitions more

4668804

toph-allen force-pushed the toph-system-caches-runtime branch from 6f26820 to 4668804 Compare March 29, 2023 16:55

toph-allen and others added 5 commits March 29, 2023 13:09

Change a note to a warning

dab6cce

rewrite GitHub style admonitions into mkdoc style

eb5c228

fix link

1bfdb00

update image name

a6f57f2

small changes to delete command and docs

b44a232

- language is case insensitive - image-name is optional, default “Local” - docs add more detail about parameters

tdstein approved these changes Mar 30, 2023

View reviewed changes

aronatkins approved these changes Mar 31, 2023

View reviewed changes

revert language variable title casing

26da4c1

Title-casing the language doesn't harm functionality, but it might confuse my future self.

mmarchetti added the hold label Apr 10, 2023

bcwu approved these changes May 1, 2023

View reviewed changes

bcwu removed the hold label May 1, 2023

toph-allen added 2 commits May 1, 2023 12:03

Merge branch 'master' into toph-system-caches-runtime

1a7ecdd

Small change to bring in-line with Connect

0e68bf1

toph-allen merged commit b510ee6 into master May 1, 2023

toph-allen deleted the toph-system-caches-runtime branch May 1, 2023 16:45

nealrichardson mentioned this pull request Jul 24, 2025

Use preview builds for testing #649

Open

		python -m unittest tests.test_main_system_caches
		pytest -m 'vetiver'

Conversation

toph-allen commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Intent

Type of Change

Approach

Automated Tests

QA Notes

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tdstein left a comment

Choose a reason for hiding this comment

Uh oh!

tdstein Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

tdstein Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

toph-allen commented Mar 27, 2023

Uh oh!

bcwu commented Mar 27, 2023

Uh oh!

toph-allen commented Mar 27, 2023

Uh oh!

aronatkins Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aronatkins Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aronatkins Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

toph-allen commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

toph-allen commented Mar 24, 2023 •

edited

Loading

toph-allen commented Apr 11, 2023 •

edited

Loading