Closed jaimergp closed 2 weeks ago
Is this issue also happening with alpha4
version? or just beta
?
You are right, this works correctly on alpha4
. At least on Linux :)
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:
libmamba 2.0.0beta1 h19be64c_0 conda-forge/label/mamba_dev
libmambapy 2.0.0beta1 py311h1564048_0 conda-forge/label/mamba_dev
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 Ctrl+C
twice. Not sure why, but this hotfix was part of a bigger fix so probably I didnt integrate something from the other fix (which was not complete yet).
I'm on it.
Correction: I was trying the wrong branch. The branch from the pr seems to work for me, one Ctrl+C does break the sleep. (I was surprised before because I used the same test script).
I also tested on main
and it still works.
@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? To clarify: I'm testing on Windows right now.
I'm on macOS and also tested on Linux via Docker:
$ conda create -n debug-libmamba-v2b1-ctrl-c --override-channels -c conda-forge/label/mamba_dev -c conda-forge python=3.10 libmambapy
Channels:
- conda-forge/label/mamba_dev
- conda-forge
Platform: osx-arm64
Collecting package metadata (repodata.json): done
Solving environment: done
==> WARNING: A newer version of conda exists. <==
current version: 24.1.1
latest version: 24.5.0
Please update conda by running
$ conda update -n base -c conda-forge conda
## Package Plan ##
environment location: /Users/jrodriguez/.local/anaconda/envs/debug-libmamba-v2b1-ctrl-c
added / updated specs:
- libmambapy
- python=3.10
The following packages will be downloaded:
package | build
---------------------------|-----------------
libmambapy-2.0.0beta1 | py310h7ae444f_0 527 KB conda-forge/label/mamba_dev
------------------------------------------------------------
Total: 527 KB
The following NEW packages will be INSTALLED:
bzip2 conda-forge/osx-arm64::bzip2-1.0.8-h93a5062_5
c-ares conda-forge/osx-arm64::c-ares-1.28.1-h93a5062_0
ca-certificates conda-forge/osx-arm64::ca-certificates-2024.2.2-hf0a4a13_0
cpp-expected conda-forge/osx-arm64::cpp-expected-1.1.0-hffc8910_0
fmt conda-forge/osx-arm64::fmt-10.2.1-h2ffa867_0
icu conda-forge/osx-arm64::icu-73.2-hc8870d7_0
krb5 conda-forge/osx-arm64::krb5-1.21.2-h92f50d5_0
libarchive conda-forge/osx-arm64::libarchive-3.7.2-hcacb583_1
libcurl conda-forge/osx-arm64::libcurl-8.7.1-h2d989ff_0
libcxx conda-forge/osx-arm64::libcxx-17.0.6-h5f092b4_0
libedit conda-forge/osx-arm64::libedit-3.1.20191231-hc8eb9b7_2
libev conda-forge/osx-arm64::libev-4.33-h93a5062_2
libffi conda-forge/osx-arm64::libffi-3.4.2-h3422bc3_5
libiconv conda-forge/osx-arm64::libiconv-1.17-h0d3ecfb_2
libmamba conda-forge/label/mamba_dev/osx-arm64::libmamba-2.0.0beta1-h2db0620_0
libmambapy conda-forge/label/mamba_dev/osx-arm64::libmambapy-2.0.0beta1-py310h7ae444f_0
libnghttp2 conda-forge/osx-arm64::libnghttp2-1.58.0-ha4dd798_1
libsolv conda-forge/osx-arm64::libsolv-0.7.29-h1efcc80_0
libsqlite conda-forge/osx-arm64::libsqlite-3.45.3-h091b4b1_0
libssh2 conda-forge/osx-arm64::libssh2-1.11.0-h7a5bd25_0
libxml2 conda-forge/osx-arm64::libxml2-2.12.7-ha661575_0
libzlib conda-forge/osx-arm64::libzlib-1.2.13-h53f4e23_5
lz4-c conda-forge/osx-arm64::lz4-c-1.9.4-hb7217d7_0
lzo conda-forge/osx-arm64::lzo-2.10-h93a5062_1001
ncurses conda-forge/osx-arm64::ncurses-6.5-hb89a1cb_0
nlohmann_json conda-forge/osx-arm64::nlohmann_json-3.11.3-hebf3989_0
openssl conda-forge/osx-arm64::openssl-3.3.0-h0d3ecfb_0
pybind11-abi conda-forge/noarch::pybind11-abi-4-hd8ed1ab_3
python conda-forge/osx-arm64::python-3.10.14-h2469fbe_0_cpython
python_abi conda-forge/osx-arm64::python_abi-3.10-4_cp310
readline conda-forge/osx-arm64::readline-8.2-h92ec313_1
reproc conda-forge/osx-arm64::reproc-14.2.4.post0-h93a5062_1
reproc-cpp conda-forge/osx-arm64::reproc-cpp-14.2.4.post0-h965bd2d_1
simdjson conda-forge/osx-arm64::simdjson-3.9.2-h420ef59_0
spdlog conda-forge/osx-arm64::spdlog-1.12.0-he64bfa9_2
tk conda-forge/osx-arm64::tk-8.6.13-h5083fa2_1
tzdata conda-forge/noarch::tzdata-2024a-h0c530f3_0
xz conda-forge/osx-arm64::xz-5.2.6-h57fd34a_0
yaml-cpp conda-forge/osx-arm64::yaml-cpp-0.8.0-h13dd4ca_0
zstd conda-forge/osx-arm64::zstd-1.5.6-hb46c0d2_0
Proceed ([y]/n)? y
Downloading and Extracting Packages:
Preparing transaction: done
Verifying transaction: done
Executing transaction: done
#
# To activate this environment, use
#
# $ conda activate debug-libmamba-v2b1-ctrl-c
#
# To deactivate an active environment, use
#
# $ conda deactivate
$ conda activate debug-libmamba-v2b1-ctrl-c
$ python ~/devel/conda-libmamba-solver/debug_ctrl_c.py
libmambapy version 2.0.0
Sleep for 5s... Try to Ctrl+C
^C^C^C^C^C^C^C^C^C^C^C%
$ conda info
active environment : debug-libmamba-v2b1-ctrl-c
active env location : /Users/jrodriguez/.local/anaconda/envs/debug-libmamba-v2b1-ctrl-c
shell level : 2
user config file : /Users/jrodriguez/.condarc
populated config files : /Users/jrodriguez/.local/anaconda/.condarc
/Users/jrodriguez/.condarc
conda version : 24.1.1
conda-build version : 24.1.2
python version : 3.9.9.final.0
solver : libmamba (default)
virtual packages : __archspec=1=m1
__conda=24.1.1=0
__osx=14.3.1=0
__unix=0=0
base environment : /Users/jrodriguez/.local/anaconda (writable)
conda av data dir : /Users/jrodriguez/.local/anaconda/etc/conda
conda av metadata url : None
channel URLs : https://conda.anaconda.org/conda-forge/osx-arm64
https://conda.anaconda.org/conda-forge/noarch
package cache : /Users/jrodriguez/.local/anaconda/pkgs
/Users/jrodriguez/.conda/pkgs
envs directories : /Users/jrodriguez/.local/anaconda/envs
/Users/jrodriguez/.conda/envs
platform : osx-arm64
user-agent : conda/24.1.1 requests/2.31.0 CPython/3.9.9 Darwin/23.3.0 OSX/14.3.1 solver/libmamba conda-libmamba-solver/24.1.0 libmambapy/1.5.6
UID:GID : 501:20
netrc file : None
offline mode : False
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 KeyboardInterrupt
, which should appear in the output.
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 libmamba
in C++ will not set the handler by default and the option can be set through the Context
constructor, see:
As you can see the signal handling is not set by default, but this is set to true
in mamba
and micromamba
.
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 true
for that option to keep the previous behavior (note that the issue here became apparent after the Context became not-a-singleton).
The rigth solution to avoid this would be to change the python API so that the context has to be manually constructed with options exactly like in C++. We planned to do that in a future release though.
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. As for the solution I'm trying: keep track of the previous handler that had been set and reuse it either when we set the option to false and/or in the handler we set.
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 https://github.com/conda/conda-libmamba-solver/pull/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.
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
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 .
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.
In C++ you have to construct yourself the context (with whatever options you want) and pass it to other functions afterwards. In python, libmambapy will create itself the context and provide it transparently, and this is mandatory as long as we want to keep the interface as before for python. If we decide we want to break it for v2.0 then you will have to change code using libmamba to create a context yourself and pass it to some other functions. While it does make more sense that way, it's also breaking a lot of interface.
This pr should make sure that the test script will work with KeyboardInterrupt
(it works at least for me on Windows)
It will need some testing.
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 Context
, then make it mandatory argument for functions that does need it in C++. This will taker time because the python tests needs to be updated accordingly. Then we'll see if we want to make transparent the context argument (by defaulting it in some ways).
Some update for this issue:
Context
object. Once created, all the internals of the module will be ready to use, but before that point they will not be usable. I am in the process of also changing the functions which require access to Context
so that it doesnt do it as a singleton internally. Although, for now, because of technical limitations, we will still prevent creating more than one Context
(the limitation doesnt exist in C++ except if you start to use mamba::Console
and mamba::Configuration
... so lets keep it simple in Python for now.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 KeyboardInterrupt
as expected.
This is not enough to close this issue though.
While investigating I found that on Linux we reset (without checking options) the signal handling mechanism even if it was not enabled before in the code handling file locking waits. This part is quite tricky and to be solved requires a rewrite of the signal handling altogether (I tried several variations of a quickfix but my conclusion, after double checking with @JohanMabille that my understanding was correct, is that it's fundamentally broken as-is and needs to be a bit different). So basically going with that rewrite (or partial rewrite) could also solve point 2.
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?
While investigating I found that on Linux we reset (without checking options) the signal handling mechanism even if it was not enabled before in the code handling file locking waits.
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).
So, in short, we still need to wait a bit for the rewrite to fully address this, right?
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.
Where in the code is that happening?
There: https://github.com/mamba-org/mamba/blob/main/libmamba/src/core/util.cpp#L877 This is called every time we finish locking a file on non-windows.
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 mamba_context = Context(ContextOptions(enable_logging_and_signal_handling = False))
and then do something, yeah
Cool, I'll wait for the next beta! Thanks again!
@jaimergp The beta is ready, could you try the workaround Context
construction and tell me if it's enough for your use case? https://github.com/mamba-org/mamba/releases/tag/2024.06.14
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 libmambapy.cancel_json_output(libmamba_context)
If I initialize without ContextOptions
, then Ctrl+C still works but the logging problem is gone. Not sure how these are related or if I'll run into problems in some edge cases if I don't pass ContextOptions
.
Ok thanks for testing! I'll check why the logging outputs when not enabled but meanwhile I think splitting the option in two so that you can disable signal handling without disabling logging seems to become important. I wanted to do that at some point, now seems to be the right time.
If I initialize without ContextOptions, then Ctrl+C still works but the logging problem is gone.
Hmm that is werid, maybe I did something wrong with the defaults...
if I'll run into problems in some edge cases if I don't pass ContextOptions.
No, when Context()
is built without a ContextOptions()
with just pass a default with the option set to true
, so I'm surprised that Ctrl + C works in that case. Anyway it should just change the option's value, no other edge cases.
The CI ran fully and now I can also see that the tests for Windows do not pass for Ctrl+C
. Linux and macOS are ok.
And speaking of logging, I see way too many lines like these:
debug libmamba No signatures available for '21cmfast-3.3.1-py310h19eaa8d_0.conda'. Downloading without verifying artifacts.
debug libmamba Adding package record to repo 21cmfast-3.3.1-py310h19eaa8d_0.conda
debug libmamba No signatures available for '21cmfast-3.3.1-py310h19eaa8d_1.conda'. Downloading without verifying artifacts.
debug libmamba Adding package record to repo 21cmfast-3.3.1-py310h19eaa8d_1.conda
debug libmamba No signatures available for '21cmfast-3.3.1-py311hc701e3d_0.conda'. Downloading without verifying artifacts.
debug libmamba Adding package record to repo 21cmfast-3.3.1-py311hc701e3d_0.conda
debug libmamba No signatures available for '21cmfast-3.3.1-py311hc701e3d_1.conda'. Downloading without verifying artifacts.
debug libmamba Adding package record to repo 21cmfast-3.3.1-py311hc701e3d_1.conda
debug libmamba No signatures available for '21cmfast-3.3.1-py38h0db86a8_0.conda'. Downloading without verifying artifacts.
debug libmamba Adding package record to repo 21cmfast-3.3.1-py38h0db86a8_0.conda
debug libmamba No signatures available for '21cmfast-3.3.1-py38h0db86a8_1.conda'. Downloading without verifying artifacts.
debug libmamba Adding package record to repo 21cmfast-3.3.1-py38h0db86a8_1.conda
debug libmamba No signatures available for '21cmfast-3.3.1-py39h72d3284_0.conda'. Downloading without verifying artifacts.
debug libmamba Adding package record to repo 21cmfast-3.3.1-py39h72d3284_0.conda
debug libmamba No signatures available for '21cmfast-3.3.1-py39h72d3284_1.conda'. Downloading without verifying artifacts.
debug libmamba Adding package record to repo 21cmfast-3.3.1-py39h72d3284_1.conda
Basically one for each artifact. I think that's way too verbose, but I'll open a separate issue for that.
The CI ran fully and now I can also see that the tests for Windows do not pass for Ctrl+C.
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 KeyboardInterrupt
exception being printed or any other indication that Python handled the exception. See this commit.
Yeah it makes sense, it would require https://github.com/mamba-org/mamba/pull/3297 to make it work as expected (KeyboardInterrupt
) with the first Ctrl+C, but I couldnt merge this yet because it only works on windows. The linux-specific code is a problem we took the time to analyze, we have a solution designed but it will take time to come and probably after the release.
Meanwhile, I guess we could solve at least the windows case by merging it. What do you think @JohanMabille
With #3329 I can make this work locally on Linux and Windows (Ctrl+C triggers a KeyboardInterrupt
):
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)
Now #3297 is merged and we plan to make a release soon, hopefully this will be easier to work with @jaimergp
Thanks @Klaim, I gave it a try and now I do get the KeyboardInterrupt
... eventually. For some reason the .solv
parsing is super slow:
DEBUG conda.conda_libmamba_solver.index:_load_repo_info_from_json_path(395): Loading conda-forge (https://conda.anaconda.org/conda-forge/win-64) from SOLV repodav
[2024-07-08 14:44:29.258] [info] [output.cpp:600] Attempting to read libsolv solv file "C:\\Users\\JaimeRodriguez-Guerr\\Documents\\GitHub\\conda\\devenv\\Window4
[2024-07-08 14:44:29.258] [info] [output.cpp:600] Expecting solv metadata : {"etag":"\"37a041dcbbafee7a2420abe9055a63e1\"","mod":"Mon, 08 Jul 2024 11:58:54 GMT",}
[2024-07-08 14:45:02.826] [info] [output.cpp:600] Loaded solv metadata : {"etag":"\"37a041dcbbafee7a2420abe9055a63e1\"","mod":"Mon, 08 Jul 2024 11:58:54 GMT","to}
[2024-07-08 14:45:02.827] [info] [output.cpp:600] Metadata from solv are valid, loading successful
~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: https://github.com/mamba-org/mamba/pull/2753#issuecomment-1739122830). 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+C
handling 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
python
and try toCtrl+C
out 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
~/.condarc