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

Add PVCache #13

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

mdavidsaver
Copy link

@mdavidsaver mdavidsaver commented Apr 15, 2021

Maybe useful in some form. I've found myself writing something like this in many CA client scripts. Meant to be used something like camontor(). Instead of Subscriptions, returns an object which can be iterated to yield snapshots of cache values whenever there is an update.

await for val1, val2 in PVCache(['pv1', 'pv2']):
    print(val1, '+', val2, '=', val1+val2)

Or with a larger number of PVs, a dictionary mode.

await for V in PVCache({'A':'pv1', 'B':'pv2'}):
    print(V['A'], '+', V['B'], '=', V['A']+V['B'])

Helper for the ~common case of acting on the
values of a group of PVs whenever any of them change.
@mdavidsaver mdavidsaver marked this pull request as draft April 15, 2021 19:16
@mdavidsaver
Copy link
Author

I clicked Create too soon. Description updated...

@Araneidae
Copy link

There is the cothread.pv.PV object in cothread. Does this cover the same ground? If so, this could readily be transferred across.

Copy link
Contributor

@thomascobb thomascobb left a comment

Choose a reason for hiding this comment

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

Seems reasonable, but I've never needed either this or cothread.pv.PV in any of my projects so I can't comment on whether it fits their use cases. Seems like this is more general (aggregate multiple PVs into a list or dict structure), while the PV (single PV) and PV_array (multiple PVs in a numpy array) are more specific. Maybe @willrogers and @Araneidae can comment on whether these are different ideas or should be merged together into one class?


return self.get_nowait()

__getitem__ = get
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work? Doesn't __getitem__ require a key?

from . import camonitor, CANothing


class PVCache:
Copy link
Contributor

@thomascobb thomascobb Apr 16, 2021

Choose a reason for hiding this comment

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

This will need a bit of work to shoehorn typing into it. I guess something like this might work:

Suggested change
class PVCache:
from typing import TypeVar, List, Dict
T = TypeVar("T")
V = TypeVar("V", Dict[str, T], List[T])
class PVCache(Generic[V[str]]):
...
def get_nowait(self) -> V[AugmentedValue]:
...

This says if you pass a List[str] as PVs, then you will get a List[AugmentedValue] from get_nowait, and if you pass a Dict[str, str] as PVs you will get a Dict[str, AugmentedValue].
It might work, depends on what mypy supports...


__getitem__ = get

async def changes(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
async def changes(self):
async def changed(self):

async def __anext__(self):
while True:
if self._first:
# (maybe) deliver initial snapshot immediately
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably needs to be an __init__ option for PVCache

@mdavidsaver
Copy link
Author

Unrelated to the code, but since I noticed your usage yesterday when I needed to enable GHA builds for my fork. fyi. It is being reported that this Action was doing nefarious things for several months this spring.

https://github.com/dls-controls/aioca/blob/853e28b6c717c14db730d5e2e3ad5719c744a9ae/.github/workflows/code.yml#L84-L87

@thomascobb
Copy link
Contributor

Yes, we had to re-roll our PyPI token because of this...

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.

3 participants