fermi-ad / acsys-python

Python module to access the Fermilab Control System
MIT License
8 stars 2 forks source link

Combine sync and dpm services #6

Open beauremus opened 3 years ago

beauremus commented 3 years ago

@kjhazelwood: Is there a benefit to having the two services separated. It looks like they can share the same Connection object, why not for the users sake combine them? This way one doesnt have to iterate over combined sync and dpm streams to monitor clock and dpm within the same script. It seems like it would be possible to allow the drf2 event string as a valid entry in dpm.add_entry.

beauremus commented 3 years ago

The idea behind packaging this way is to limit how much a developer includes in their code. For example, most people aren't using sync. I think in the case of sync, it's a small amount of code but we were thinking it solves a potential future bloat problem. That being said, I'm not for pre-optimization so maybe services can be more integrated until we need to break them out.

I don't know that combining them in the way you suggest solves the problem. They are different services and open separate channels. DPM talks to DPMs and Sync talks to SYNCD.

It wasn't clear to me why someone would want both sync and DPM until I asked you. Our thinking is that DAQ is driven by events and that gives you the relevant information.

Trying to take something away from this issue, I think a unified interface to iterate over all requested data is a nice idea that we should consider for the next major version.

beauremus commented 3 years ago

@rneswold: The idea behind packaging this way is to limit how much a developer includes in their code.

Exactly. The control system has a lot of protocols which are the building blocks for a controls application. The acsys packages was a way to use what you need.

I created the SYNCD service because there was a need: Wally wanted to know when a particular event occurred so he made a DPM request to read a device on the event of interest and simply threw away the device data. Along with being slightly wasteful of front-end CPU and network bandwidth, his script would break if the front-end went down. So I created the service, which runs on a pool of nodes, so he wouldn't be dependent on a front-end.

But ACNET is a transport for many, many protocols. We're supporting two of them for Python, but others can be added. Maybe we could be better at hiding the Connection object. I wish Python had better built-in combinators for async generators. Maybe we'll have to add some in acsys...

kjhazelwood commented 3 years ago

I dont have any issues with having to import sync and dpm to use both, I agree that compartmentalizing is the way to go. I have a lot of use cases to monitor the clock while sampling readings at the same time. acsys.run_client() passes a Connection object to a async method, if I want to monitor both clock and readings I need to use the same Connection object to instantiate both and then combine streams because iterating over either separately is synchronous. Im not sure what the best approach would be but I dont think creating multiple connections is the way to go (unless DPM has smarts to return to me the same Connection) so maybe allow the Connection object to be passed around and allow the user to pass it to a threadpool model so multiple things could asynchronously share the connection?

rneswold commented 3 years ago

Yes, the con object can be passed around. Something like:

async def handle_clock(con):
    async for ii in acsys.sync.get_events(con, ['e,8f']):
        # do stuff because event fired

async def handle_dpm(con):
    async with DpmContext(con) as dpm:
        # set up DPM requests

        # loop through replies
        async for ii in dpm.replies():
            # do stuff with DPM data

async def main(con):
    asyncio.gather(handle_clock(con), handle_dpm(con))

which works when your handling of events has nothing to do with handling data replies.

The con object is your one connection to ACNET. The different modules (sync, dpm) make requests using this connection and they won't interfere with each other.

beauremus commented 3 years ago

@kjhazelwood I'm guessing your current testing will answer this question but is this still an issue. Does @rneswold 's solution work for you?