Heron-Repositories / Heron

A python only framework for connecting hardware and closing loops
MIT License
53 stars 6 forks source link

Half a solution to a problem for non-windows users and some thoughts on the project #9

Closed timhuff closed 1 year ago

timhuff commented 1 year ago

Hey, first off, I love this architecture. I think there's a lot of potential here. Especially in terms of using this in conjunction with a service like AWS's Lambda for fast, cheap processing when it's needed. I have a few notes and suggestions and an apparent bug fix. I don't have the time at the moment to put together anything proper but I figured I'd pass along the knowledge.

That said, I spent the past day trying to get this to run on my mac. I don't know if I missed it in the docs (the docs seem a bit inconsistent, by the way... it made installation very confusing), but I was stuck for hours because I didn't realize you needed to highlight the part of the graph that you wish to run. Perhaps add something about that? (also, it seems that if no node is selected, it should run all of them?). Anyway, not really why I'm here.

Posix Issue / Fix (?)


I was never able to get the system to output anything but I think I was getting close. A bug that hit me right off the bat is subprocess.CREATE_NEW_PROCESS_GROUP does not exist on POSIX systems. It's used in your code at 3 locations and the program no longer errors out if I use the following pattern.

        kwargs = ({ 'preexec_fn': os.setpgrp, 'shell': 'bash' }) if os.name == 'posix' else ({ 'creationflags': subprocess.CREATE_NEW_PROCESS_GROUP })
        self.process = subprocess.Popen(arguments_list, **kwargs)

My primary language is JS so I'm not 1000% on if the posix kwargs dictionary is set up as it should be. But I've confirmed that when editor is closed, the child processes no longer exist.

After making that change, I was able to run it but nothing happens. I finally threw in the towel because, while what you have is good, I don't need the distributed computing piece for now and setting up the DSP pipeline functionality to run locally shouldn't be too hard to quickly put together.

Suggestions


A few quick thoughts that you may have already considered but I wanted to throw them out there just in case:

1) I see you're using zeromq for messaging between nodes. Have you considered zerorpc instead?

Support for streamed responses - similar to python generators - makes zerorpc more than a typical RPC engine. Built-in heartbeats and timeouts detect and recover from failed requests. Introspective capabilities, first-class exceptions and the command-line utility make debugging easy.

It seems to me that those elements of the system are the most difficult: the remote function calls. But it seems you're doing it all manually with zeromq and missing out on the support you could be provided by leaning on that project. For example, the above Posix issue wouldn't exist with it.

2) I think there's also potential in utilizing RxPY for logic flow within the graph. I have a few reasons for believing this.


Well I have to get back to it but I'll be working on a rudimentary DSP pipeline that incorporates RxPY and DearPyGui's networks to produce a program that enables the user to construct DSP pipelines to be ran locally. Due to the nature of RxPY, it should be fairly straight forward to extend this functionality across the network using zerorpc. I'll need to talk to my company about if they'll allow me to open source the code. Pretty sure they'd be fine with it as long as I simply shared the architecture and not any of our filters or anything.

georgedimitriadis commented 1 year ago

Hi timhuff,

Thank you for the issue and the suggestions.

I finally had the time to have a look at it (and setup my linux environment to try and solve it).

You came very close to the solution which is

kwargs = {'start_new_session': True} if os.name == 'posix' else \
        {'creationflags': subprocess.CREATE_NEW_PROCESS_GROUP}
forwarders = subprocess.Popen(['python', os.path.join(path_to_com, 'forwarders.py'), 'False', 'False', 'False'], **kwargs)

Apparently the 'preexec_fn': os.setpgrp property kind of deprecated (and for some reason won't work in my setup), but the 'start_new_session': True will do the job just fine. Again, thank you very much for the feedback.

Regarding using Rx, Heron is inspired by Bonsai which is an effort in C# fully implementing your idea of Rx for message passing. I have used Bonsai extensively and coded a bit for it but I developed Heron because I found it rather hard to actually work with. This is a big discussion but I found that your 3rd and 4th points were not holding true (at least for me). The flow of info becomes a giant spaggeti for anything other than trivial examples and I found writting Operators frastrating in the best of times (probably my brain is not 'wired' appropriately).

Good lack with your efforts and let me know if you have something that you are happy with and is open source.