Add basic support for asynchronous reads #403
Conversation
|
@natezb thanks for the PR it looks promising. I will take time later to review it in details. However, it may take some time to merge it since the yield from syntax was introduced in Python 3.3 and cannot be easily implemented in Python 2. Currently PyVISA supports Python 2.7 and 3+. After the next release (for which I do not have a well defined timeline, but this summer or before hopefully), we will switch to 3.5+ since Python 2 EOL is 01-2020. |
|
Yeah, I noticed exactly the same thing, as my first two commits produced a broken build. My third commit separates out the code using |
|
A few years ago I was toying with something like this (yours looks better, by the way). And I have a suggestion. Why not creating a separate package (async-pyvisa, or something like this)? People using async are still a niche (a growing nich, but still a niche). An pip installing is simple enough. In this way you will be able to evolve that package as frequently as you can until you feel satisfied. |
There was a problem hiding this comment.
Overall this looks really good. I am not familiar enough with async to check the logic (but I will try to do some reading). From my point of view, I would prefer to have all read method to have equivalent async method. The main question here is how to avoid duplication, so far I do not have a good answer for this one.
@hgrecco I am not a big fan of having another pyvisa package with the added maintenance it represents given the limited time the maintainers have. Since this cannot go in before the end of the summer, I think we can discuss and experiment in this PR before merging. Furthermore I am fine with marking the API as unstable for 6 months after adding it.
|
|
||
| job_id, _ = self.visalib.get_attribute(ctx, constants.VI_ATTR_JOB_ID) | ||
| count, _ = self.visalib.get_attribute(ctx, constants.VI_ATTR_RET_COUNT) | ||
| self.visalib.close(ctx) |
There was a problem hiding this comment.
I would prefer to see this in a try/finally block to be sure we always close the context.
| warnings.warn("read string doesn't end with " | ||
| "termination characters", stacklevel=2) | ||
|
|
||
| return message[:-len(termination)] |
There was a problem hiding this comment.
I would prefer to be complete here and hence have also async_read_bytes, async_read_ascii_values, async_read_binary_values. I am not sure what is the best way to limit code duplication here however.
|
Any update on this? |
|
No, but internal refactoring should make this easier now. (but also created some conflicts). I do not have experience with async and I would prefer somebody else to have a look honestly. Feel free to start another similar PR if you have a definite use case par it. |
|
Hi folks, I'm quite interested in this. I've had a look at the pull request and have tried some experiments myself and have a few comments. First off, the code from @natezb is very interesting and he was way ahead of his time, implementing this 4 years ago, impressive! Async/Await has come a long way in the intervening years and I think it would be a great time to try another run at this. I note you require Python 3.8, which has very robust asyncio features. There's one main thing I'd like to bring up about this old proposal, and that's the tight loop in the center of the while True:
yield
msg, job_status = self._check_async_read(timeout=0)
if msg is not None:
return msg, job_statusThis means that awaiting an async read is basically a As I understand it, at the VISA level, we are looking to be notified of the Queue mechanismPoll the queueasync def _async_read_polling(self):
while True:
msg, status = self._check_async_read(timeout=0)
if status is DONE:
return msg
asyncio.sleep(POLL_TIME)Where Wait on queue in a thread or process poolSee documentation for run_in_executor async def _async_read_executor(self):
loop = asyncio.get_running_loop()
msg, job_status = await loop.run_in_executor(executor, lambda: self._check_async_read(timeout=a_long_time))¶
return msg, job_statusThis uses an executor to run the blocking Callback mechanismWe can get away from any blocking calls at all by using the callback mechanism. The callback must have a reference to the main asyncio event loop and some kind of asyncio.Queue or asyncio.Event that it can set when the VISA_IO_COMPLETION eve 8000 nt fires. The callback is run from outside of the main event loop, so the synchronization should be done with loop.call_soon_threadsafe() async def _async_read_callback(self):
queue = asyncio.Queue
handler = self._make_callback(queue, asyncio.get_running_loop())
self.install_handler(VI_EVENT_IO_COMPLETION, handler)
buf, *_ = self.visalib.read_asynchronously(self.session, self.chunk_size)
(job_status, ret_count) = await queue.get()
msg = bytearray(buf[:ret_count])
self.uninstall_handler(...)
return msg, job_statuswhere the _handler_for_async_readmethod is something like def _handler_for_async_read(self, queue, loop):
def _handler(session, event_type, event_ctx, user_handle):
job_status, _ = self.visalib.get_attribute(event_ctx, VI_ATTR_STATUS)
ret_count, _ = self.visalib.get_attribute(event_ctx, VI_ATTR_RET_COUNT)
loop.call_soon_threadsafe(
lambda: queue.put_nowait((job_status, ret_count))
)
return _handlerConceptually, I quite like this because it avoids blocking, polling, and creating any extra threads on our end. I have a proof-of-concept working and could start another pull request if there was interest in discussing that. I am NOT an expert on low level VISA stuff so I will probably need help to make sure I'm following best practices there. Any thoughts on these options or next steps? |
|
Ok, just some extra information about those options. I threw together a very ugly implementation: And benchmarked them, doing "*IDN?" on a real instrument via the IVI Visa library. The bottom line is: Async reads via callback added ~0.4ms per read, compared to a synchronous read Thus, I think the callback method is significantly faster. |
|
So since my last comment I gained experience with async, but did not find time to dive back into this. I am not surprised to see the callback mechanism outperforming the poll based method. The last time I gave async support some though my concern was that to be useful we would have to duplicate all methods. Async makes most sense to use for binary data Honestly if you want to take this forward, I would like to see a detailed plan (PEP-like) addressing the above concerns and what solution you suggest. |
This PR added attributes related to IO_COMPLETION events, along with some methods for performing generator-based asynchronous reads.
I've tested this by pulling data from two oscilloscopes simultaneously, and it has worked well.