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

REF: unnamed pipe -> named pipe (FIFO) for SubprocessTao #98

Merged
merged 6 commits into from
Oct 1, 2024

Conversation

ken-lauer
Copy link
Collaborator

@ken-lauer ken-lauer commented Sep 16, 2024

Context

Based on #95 as that PR cleans up a lot of subprocess-related handling. It needs to be decided on/merged first.

  • We found instability when using SubprocessTao in multiprocessing.Pools
    • Occasionally this would raise OSError: Bad file descriptor (2-10% of the time)
    • It seems as if there are race conditions with os.pipe() and inherited file descriptors in subprocesses, such that they can fight with one another. It never happens on the first initialization from what I've seen.
  • Utilizing named pipes, as in this PR, appears to not have these issues
    • In multiple local runs, this has had a 0% failure rate (1000s of runs, 10 multiprocessing subprocesses)
  • I think this is a bit cleaner of a solution
    • Even if we figured out how to address the pipe() issue, I think I'd like to move forward with this anyway
  • Bonus: custom commands for SubprocessTao
    • Run importable callables in the subprocess and get the result by way of tao.subprocess_call(function, **kwargs)
    • The tao keyword argument is inserted automatically

@ken-lauer ken-lauer changed the title WIP/REF: unnamed pipe -> named pipe (FIFO) for SubprocessTao REF: unnamed pipe -> named pipe (FIFO) for SubprocessTao Sep 17, 2024
@ken-lauer ken-lauer marked this pull request as ready for review September 30, 2024 20:03
@ken-lauer
Copy link
Collaborator Author

This is working well in my own tests and also a suite of much heavier testing by @electronsandstuff.
It's ready for review.

If all looks good and we merge this, I think it's about time for a new tag!

@electronsandstuff
Copy link
Contributor

OK, I have run a multiple core example with the modified SubprocessTao class doing the heavy work for eight hours now with good results. The new version seems to be working well.

@ChristopherMayes ChristopherMayes merged commit d0b65c5 into bmad-sim:master Oct 1, 2024
10 checks passed
@ken-lauer ken-lauer deleted the ref_subprocess_fifo branch October 1, 2024 22:28
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