Skip to content
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

Asynchronous skeleton generation and instituted low/high PubSub skeleton generation #298

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kebwi
Copy link
Collaborator

@kebwi kebwi commented Feb 7, 2025

CAVEclient:SkeletonClient.get_skeleton() has a new boolean async_ parameter. If false, the function performs as before, blocking until a skeleton is generated and returned (which may be very quick if the skeleton is in the cache). If async_ is passed as true, this function behaves identically from the user's perspective, blocking until a Skeleton is available and returned. In both cases, if a skeleton is in the cache already, it will probably return in a few seconds. In both cases, if a skeleton is missing from the cache, a skeleton is generated and then returned. However, in the new asynchronous case, skeleton generation is performed by publishing a request to the PubSub queue and then polling the cache periodically (every 5s) until the skeleton is available. The polling is performed on the server by SkeletonService, not by the client ("over the wire" at a distance). It is debatable what utility this new method offers, but it supports high and low priority messaging. Skeletons generated this way go into the high priority queue.

Note that the new low and high priority queues depend on a new version of messaging-client, which isn't officially deployed yet. I have forced it into LTV5 by hacking SkeletonService:requirements.txt to pull from the relevant Github branch, but it is not published to PyPi yet. The same approach could be taken on minniev6 of course, and would safely only affect SkeletonService since each repo/docker-image/kube-pod operates in isolation (all other repos using messaging-client would continue to use the PyPi release), but ultimately, I'm unsure of the correct order of operations to fully deploy these various tools.

CAVEclient:SkeletonClient.generate_bulk_skeletons_async() works as before, but publishes requests to the low priority queue. Any skeletonization requests issued using the new option described above for get_skeleton() will supersede those generated using generate_bulk_skeletons_async().

CAVEclient:SkeletonClient.get_bulk_skeletons() has deprecated the generate_missing_skeletons parameter. Although left in place for backward compatibility, the parameter is now ignored. Any skeleton that is missing is now published as a request on a the high priority queue with no corresponding skeleton return by this function. At some later time (presumably within about 1 minute) the skeleton will become available and can be retrieved simply be recalling this function with any root ids that did not previously produce skeletons. However, there is no practical way for the user to know if and when the skeleton is available without either calling skeletons_exist() or simply recalling this function in an effort to retrieve the skeleton.

Note that the last change mentioned above, get_bulk_skeletons() can effectively replace other channels into the skeleton service, most notably get_skeleton(), thereby simplifying the service (and likewise its front-end in CAVEclient). That would be the next step.

@kebwi kebwi requested review from ceesem and fcollman February 7, 2025 20:46
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 16.21622% with 31 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
caveclient/skeletonservice.py 16.21% 31 Missing ⚠️
Files with missing lines Coverage Δ
caveclient/endpoints.py 100.00% <ø> (ø)
caveclient/skeletonservice.py 52.05% <16.21%> (-1.58%) ⬇️

@ceesem
Copy link
Collaborator

ceesem commented Feb 8, 2025

I don't think I understand the need for the async_ parameter if the user-side behavior is identical. It feels like the new behavior is more like what we want to move toward, so I would be inclined to make that the new implementation so we can be sure it works well in all cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants