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

DM-40638: Add AsyncMultiQueue data structure #195

Merged
merged 6 commits into from
Sep 7, 2023
Merged

DM-40638: Add AsyncMultiQueue data structure #195

merged 6 commits into from
Sep 7, 2023

Conversation

rra
Copy link
Member

@rra rra commented Sep 5, 2023

Add an asyncio multiple writer, multiple reader queue, and convert the Kubernetes mock to use it instead of implementing its own. This data structure is also used in the JupyterHub REST spawner and in the Nublado lab controller.

@rra rra requested a review from jonathansick September 5, 2023 19:40
@rra
Copy link
Member Author

rra commented Sep 5, 2023

Sphinx plus the extensions we use is refusing to generate any method documentation for AsyncMultiQueue and I cannot figure out why. If you have a few minutes, I'd love another pair of eyes on it to try to figure out what I'm doing wrong or what weird edge case I'm running into.

Add an asyncio multiple writer, multiple reader queue, and convert
the Kubernetes mock to use it instead of implementing its own. This
data structure is also used in the JupyterHub REST spawner and in
the Nublado lab controller.
@rra rra force-pushed the tickets/DM-40638 branch from 3fde461 to ecb6274 Compare September 5, 2023 20:20
@jonathansick
Copy link
Member

I'm not sure about the docs build. I tried simplifying the class; I tried upgrading to Sphinx 7... so far nothing seems to be working.

@jonathansick
Copy link
Member

Dropping sphinx_autodoc_typehints didn't help either.

@rra
Copy link
Member Author

rra commented Sep 5, 2023

It feels like an automodsumm problem, but I can't figure out what would be special about this class to make it think it shouldn't generate method documentation.

@jonathansick
Copy link
Member

It seems that the Generic superclass is the issue. It's interesting that the PydanticRedisStorage class works fine, but it's generic has a bound?

@rra
Copy link
Member Author

rra commented Sep 6, 2023

I was wondering about that, but that was exactly my confusion. I added a bound of BaseModel locally and rebuilt the docs and still, no method documentation at all, so I'm not sure what's different between the two classes.

@rra
Copy link
Member Author

rra commented Sep 6, 2023

Ah, it is a bug but I found a workaround. I have to flag it with :inherited-members: or it tries to use __slots__ to discover the class members and Generic also uses slots in some weird way that apparently sometimes confuses it.

rra added 4 commits September 6, 2023 08:07
sphinx_automodsumm's ability to discover the methods of a class
appears to have some bug with some but not all classes that inherit
from Generic that seems to be related to the use of __slots__ to
determine the methods of the class. Tagging safir.asyncio with
:inherited-members: changes the code to use dir() to retrieve all
members instead, which avoids the problem.
Replace the mention of the clear method with a proper cross-reference
now that documentation generation is working correctly.
There's no need to set the trigger when adding a new iterator, since
the first thing the iterator does is unconditionally clear the
trigger and look at the contents of the queue.
In order to support the way another version of this data structure
was used in the Nublado REST spawner, add a way to mark the queue
contents as complete without clearing it. This supports the use case
of starting a reader after the queue is complete and therefore should
signal iterators to end, allowing it to see all of the captured data
and then immediately exit without further waiting.
Copy link
Member

@jonathansick jonathansick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very neat class.

On further thought, close is a more obvious name for this method
than end, making it clear that no further data can be sent similar
to a socket close. Add more documentation for how close and clear
work.
@rra rra merged commit 20d59ab into main Sep 7, 2023
@rra rra deleted the tickets/DM-40638 branch September 7, 2023 22:40
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