search

pydicom / pynetdicom

A Python implementation of the DICOM networking protocol
https://pydicom.github.io/pynetdicom
MIT License
513 stars 180 forks source link

Move PyNetDicom to twisted or another async io framework #527

Open chsasank opened 4 years ago

chsasank commented 4 years ago

In #491, @scaramallion discussed removing python threads and using aysnc io/networking framework like twisted. We were looking to use pynetdicom in our project and we arrived at the same conclusion as @scaramallion. We have gone through twisted and it has great support to create protocols like SSH.

I know DICOM is lot more complicated than say, SMTP - All the more reason to not write low level code. We should leverage as much python ecosystem as possible.

Is your feature request related to a problem? Please describe.

Right now, pynetdicom depends a lot on python threads. It has code to handle race conditions, code to do transport at low level etc. Interface to use pynetdicom is lot difficult to use compared to other protocols because of this.

Describe the solution you'd like

There's a lot of code in pynetdicom whose functionality can be offloaded to twisted or some other framework. Doing this improves maintainability, readability and testability of the repo.

Describe alternatives you've considered

Twisted is great. It has built in support for lot of features required to write very clean code for new protocols. Twisted comes with its own event loop reactor, tcp/ip support allowing us to write proper servers and clients and lot more

These paradigms map fairly well to things like association, SCU, SCP etc.

Another advantage with twisted is we can write this interface called ASGI and then start using proper web frameworks like Django. Django channels has done just this using their channels and daphne projects for websocket protocol.

Additional context

We're willing to do most of the heavy lifting if maintainers can help us with the design that is acceptable to them.

cc: @yvd

scaramallion commented 4 years ago

I've done very little research so far into what I'd like to use to replace threading so I don't have an opinion on your proposal one way or the other but twisted is definitely one of the candidates. I think it's great that you want to work on it though and I'm happy to provide advice if you want it.

chsasank commented 4 years ago

Great!

I'm happy to provide advice if you want it.

Awesome. We'd certainly want some advice. Although we have a lot of background on web, our idea of dicom networking is limited. It'd certainly help if you can let us know where we can start from pynetdicom point of view.

scaramallion commented 4 years ago

How would ASGI work with the DICOM TCP protocol? As I understand it, ASGI only applies to HTTP or websockets, not TCP.

chsasank commented 4 years ago

ASGI is designed to be work for both synchronous and asynchronous protocols - unlike WSGI. Although most of the protocol is written for HTTP and websocket, we can extend it to any protocol. ASGI is just a simple reference (as opposed to a library) about how protocol functions should look like.

Having said that, ASGI is notch above what we have to do (in perspective of the OSI stack). We have to first tackle replacing python threading with twisted in pynetdicom. ASGI interface is fairly easy to build on top of this.

scaramallion commented 4 years ago

Hmm, OK, thanks for the explanation. It does look like it'd be a nice extra though.

chsasank commented 4 years ago

We have suggested twisted primarily because of its documentation for tcp/ip protocols. However, twisted has become somewhat 'oldschool'. I'm no expert in systems programming but trio seems quite interesting. What do you think is better - trio or twisted - for our use case?

scaramallion commented 4 years ago

I have no real experience with either so no idea. I'm only just starting to experiment by attempting to write a cut-down association client and server using the various options.

chsasank commented 4 years ago

When I get time, I'll check trio out and I'll let you know my findings. I highly recommend the first tutorial of twisted: https://twistedmatrix.com/documents/current/core/howto/servers.html

moloney commented 3 years ago

I have released a related package dcm that provides a high level asyncio API on-top of pynetdicom (basically entirely contained in the net module).

I run the pynetdicom routines in worker threads and use janus to provide a queue that provides both sync / async access. This obviously isn't ideal, but actually works reasonably well. Having an async API in pynetdicom itself would be great, but I do hope that compatibility with the asyncio eco-system is a priority. In this regard I think Twisted might be better than Trio (if an additional library is used), but I also see that Trio has an escape hatch in trio-asyncio so maybe it is a non-issue.

One lesson I have learned from writing this code is that in the world of Python async, context managers are critical to doing cleanup due to the issues outlined in PEP 533.

phistep commented 1 year ago

What is the status here? https://github.com/pydicom/pynetdicom/pull/642 seems to have gone stale, https://github.com/RadiotherapyAI/aiopynetdicom has not been updated in two years, 2023 Python asyncio has improved quite a bit since 2021. Any plans to continue this effort? Any way I could help out?

scaramallion commented 1 year ago

I would say it's definitely stale. I support the effort if someone wants to try to implement it, but I don't think I can contribute much time to doing so.

medihack commented 1 year ago

I am also quite interested in that. Currently, we use pynetdicom in a separate thread in an asyncio application and are pretty happy with it. Performance is good enough, and the reliability is excellent. I wonder a bit what to expect from a re-write using asyncio. More performance (any ideas on what scale?) or improved code maintenance (the asyncio streaming API is quite nice)?