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

Add version of YAMLConfigManager that allows to run in a non-main thread #63

Open
zz1874 opened this issue Jan 26, 2024 · 8 comments
Open

Comments

@zz1874
Copy link

zz1874 commented Jan 26, 2024

This is co-authored by @simeoncarstens

We are implementing an HTTP API for looper (see pepkit/looper#441), and since the HTTP API should be non-blocking, we need to run looper in a separate thread. The only thing preventing us from that is the YAMLConfigManager class that uses signal to capture Ctrl-c inputs, which only works in the main thread.

Could we add a version of YAMLConfigManager for non-interactive use that doesn't use signal?
The commits we've made to run looper in a separate thread are pepkit/looper@bd73e0a and pepkit/looper@6483045, this environment variable solution might not be appropriate, and we may think of some other solutions.

@nsheff
Copy link
Member

nsheff commented Jan 26, 2024

I'd recommend not using an env var; instead, add a param like thread_safe to the object, and set it to False by default...

then turn on that param when instantiating the object from within looper.

@nsheff
Copy link
Member

nsheff commented Jan 26, 2024

oops wrong issue

@simeoncarstens
Copy link
Collaborator

@nsheff yep, I agree - the env var was really just to "make it work" without actually committing to changing any yacman code. I'll put up a coupe PRs then that implement a thread_safe option and propagate it through to looper / pipestat.

@nsheff
Copy link
Member

nsheff commented Jan 27, 2024

@simeoncarstens I misspoke earlier when I talked about a read( ... ) context manager not requiring signal -- in fact, the read context manager also creates a lock, and so it would also require the signal call. So, my original idea doesn't solve the issue, unfortunately.

But, I have another idea:

what is the error type Python spits out if you try to call signal from a thread? Could I just say:

try:
    signal( ... )
except UsedSignalInThreadException as e:
    pass  # don't register signal handlers when in a thread.

I think if yacman did this (for whatever the right exception type is, I made up UsedSignalInThreadException), then it would work...

@simeoncarstens
Copy link
Collaborator

@nsheff sorry for the late reply! I'm not sure I understand why the read(...) context manager also needs to create a lock. If, for now, we have no way of looper actually changing a config file, then the file can be changed only manually by a user, and in that case I guess locking would be impossible.
That being said, I think your suggestion would also work - we could just catch this exception and not register the signal handlers, if not possible. But that of course means that there could be cases where handlers are inadvertently not registered, so we should at least output a warning to the logger.

@nsheff
Copy link
Member

nsheff commented Jan 30, 2024

no problem

can you let me know exactly what exception is raised? It will reduce the probability of the inadvertent problem you mention if we are specific about the exception.

@simeoncarstens
Copy link
Collaborator

Sure! It raises ValueError: signal only works in main thread of the main interpreter.

@nsheff
Copy link
Member

nsheff commented Oct 9, 2024

This was handled in ubiquerg.ThreeLocker, which is what the future yacman will use, so this problem will be solved in version 1

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

No branches or pull requests

3 participants