v2: Ctrl+C / SIGINT / KeyboardInterrupt handling not working #3271
Comments
|
Is this issue also happening with |
|
Sorry, I tested the wrong file. I can reproduce the original report with alpha4 too. IOW, Ctrl+C doesn't work there either. |
|
Thanks for #3285 and the beta1 release. Unfortunately, it still doesn't fix the reproducer script for me. IOW, this still gets hung: import time
from libmambapy import Context
# This line below doesn't have any effect;
# it can be commented out and the result is the same
Context.use_default_signal_handler(False)
print("Sleep for 10s... Try to Ctrl+C")
time.sleep(10)With versions: Is that how I am supposed to use it? 🤔 Additionally, is #3285 accounting for Windows too? |
|
Yes that's the way to use it, so it is still broken :s |
|
I'm a bit surprised as it was working well for me but now I see that it only works when doing |
|
Correction: I was trying the wrong branch. I also tested on @jaimergp Looks like something is different between our testing cases, could you give more info about your setup so that I can attempt to reproduce it? |
|
I'm on macOS and also tested on Linux via Docker: I haven't tested the beta1 on Windows, but what I observed there during the early testing is that the process is finished after a couple of Ctrl+C, but abruptly. The correct behavior is that Python catches the signal and raises |
|
It's possible that the hotfix is replacing a handler introduced by python, I'll try to do that better. |
|
Is it possible to have the C++ signal handler be opt-in via the application layer? IOW, that micromamba sets the handler explicitly, but that otherwise there's no signal specific logic set in libmamba(py)? |
|
Currently using As you can see the signal handling is not set by default, but this is set to The issue here, indeed, is that in libmambapy we didnt want to break the way you use the library in python, so we had to default Meanwhile I'm trying to fix the issue without changing that API for the immediate time, although if you think you can wait for a future version I can stop and focus on the bigger better solution but it will only be available in a future minor release. |
|
Thanks for the details! v2 contains plenty of API breaking changes already so one more would be acceptable and not a problem. If you'd rather go that way, I'm fine waiting for that and sticking to v1 for the time being. Actually I think we are not explicitly initializing the context in conda/conda-libmamba-solver#457 anymore because we explicitly configure the Database and Solver with all the options (we do take the default values from conda's context, not libmamba's). That said, if the interim solution is within acceptable reach (it doesn't take you hours of work), I'll be happy to assist with the debugging <3 As an aside question: should we be initializing the libmamba context ourselves? I was surprised to see that I still needed to set the context bool for the signal handlers, so I don't know if it's still being used behind the scenes for something else. |
I'll see what we can do, although dont hold your breath for v2.0 because I suspect this change might also break a lot of mamba python tests .
In C++ you have to construct yourself the context (with whatever options you want) and pass it to other functions afterwards. |
|
This pr should make sure that the test script will work with Other than that we agreed with others at Quantstack to go with just breaking the interface and see after if we need to provide help or an api layer for helping with current users integration. The first step is to just make available in python the creation of the |
|
Some update for this issue:
Currently with this branch on my linux and windows I can do: import time
from libmambapy import Context, ContextOptions
mamba_context = Context()
print("Sleep for 10s... Try to Ctrl+C")
time.sleep(10)In this case we get the default normal behavior where mamba handles the signals, which is the issue here. So now the workaround would be: import time
from libmambapy import Context, ContextOptions
mamba_context = Context(ContextOptions(enable_logging_and_signal_handling = False))
print("Sleep for 10s... Try to Ctrl+C")
time.sleep(10)Which prevents mamba from even starting to take the signal handling. Ctrl+C does raise a
The rewrite would be to have a clear separation beween an interface allowing to know when a special event occurs, with a callback system + storing state or something similar - what we have is close to this but not exactly), and how the event is implemented to detect (here signal handling for example. Because different parts of the code wants to know if we have been interrupted, but these parts of the code also try to do things related to how the detection is being implemented. So essnetially this needs to be better organized and that should help fix the issue (any source of interruption becomes an interruption for whatever is interested in interruptions, nothing more complicated). |
|
Thanks for the explanation @Klaim! So, in short, we still need to wait a bit for the rewrite to fully address this, right?
Where in the code is that happening? Because maybe we are lucky in conda-libmamba-solver and we are not invoking that part (e.g. networking). |
We'll see after I'm done with the python api changes and the fix of on linux, we need to evaluate if it would be long to rewrite but I might do something that relies on part of the current code. Not sure yet.
There: https://github.com/mamba-org/mamba/blob/main/libmamba/src/core/util.cpp#L877 |
|
Hm, the only IO we rely on is writing the SOLV file. If we are lucky and that function is not called there... maaaybe it works? |
|
You'll have to check if Ctrl+C still works after you set |
|
Cool, I'll wait for the next beta! Thanks again! |
|
@jaimergp The beta is ready, could you try the workaround |
|
Hi! I checked and it works now. I observed an interesting interaction with the logging system, though. If I initialize the context like this: libmamba_context = libmambapy.Context(
libmambapy.ContextOptions(enable_logging_and_signal_handling=False)
)Ctrl+C works, but I also get verbose logging to STDOUT (not STDERR). This breaks JSON output even if I cancel it via If I initialize without |
|
Ok thanks for testing!
Hmm that is werid, maybe I did something wrong with the defaults...
No, when |
|
The CI ran fully and now I can also see that the tests for Windows do not pass for And speaking of logging, I see way too many lines like these: Basically one for each artifact. I think that's way too verbose, but I'll open a separate issue for that. |
Ok, I looked into it and it actually passes... sort of. I need to send two Ctrl+C signals and then the processed is killed immediately. However, the signal is not propagated back to Python so we don't see the |
|
Yeah it makes sense, it would require #3297 to make it work as expected ( |
|
With #3329 I can make this work locally on Linux and Windows (Ctrl+C triggers a import time
from libmambapy import Context, ContextOptions
mamba_context = Context(ContextOptions(enable_logging = True, enable_signal_handling = False))
print("Sleep for 10s... Try to Ctrl+C")
time.sleep(10) |
|
Thanks @Klaim, I gave it a try and now I do get the ~30s for conda-forge on win-64. This call is blocking so it takes a while for Python to recover control to raise the KeyboardInterrupt. I don't know if this performance is a regression but my experience in Unix systems is that .solv loading is almost instantaneous. edit: Turns out SOLV files are known to be very slow on Windows so we are better off without them (src: #2753 (comment)). That's ok with me. |
Troubleshooting docs
Anaconda default channels
How did you install Mamba?
Other (please describe)
Search tried in issue tracker
ctrl c handling
Latest version of Mamba
Tried in Conda?
Not applicable
Describe your issue
In the latest beta,
Ctrl+Chandling from Python doesn't work correctly. In v1, we addedContext.use_default_signal_handler(...)as a workaround, but this code doesn't seem to work in v2 anymore.This simple script reproduces the problem. Run it with
pythonand try toCtrl+Cout of it:It just gets stuck until the end of the 10s sleep:
If you remove the libmambapy imports, the outcome is:
mamba info / micromamba info
Logs
No response
environment.yml
N/A~/.condarc
N/AThe text was updated successfully, but these errors were encountered: