Fix initializing two recordings with the same recording id causing SDK hangs

[AhmedMousa-ag]

Related

• Closes Initializing twice with same recording id hangs the SDK #9948

What

• The hang in the issue Initializing twice with same recording id hangs the SDK #9948 is caused when trying to insert duplicated recording, and since we don't need this behavior, just avoid inserting the new recording. The real reason for hang not known yet, but it's not replicated in PyTests.

• Moved the py_rerun_warn function from rerun_py/src/dataframe.rs to rerun_py/src/utils.rs for readability.

• Added a new function that does both logging the warning using re_log::warn macro and PyErr::warn to detect the warning in python as using re_log::warn macro alone isn't enough to detect the warning in python, and using PyErr::warn doesn't log the warning as per your code convention. I suggest to use this function for other warnings in the python bridge

• Added a unit test for detecting the warning of duplicated recording stream. Unforntually the hanging behavior can't be replicated in PyTests, still not sure why.

[Wumpf]

thanks for looking into this! Needs some more polish though

[Wumpf]

Running python tests with pixi run py-test shows that a lot of tests are now broken because of the emitted warning. Haven't looked through the exact cause on all of them whether the warning should be handled, or strict mode is leaking etc. but this clearly needs some attention

[AhmedMousa-ag]

Which pixi version are you using? it says in the toml file you're using 0.34.0 but the pixi.lock refer to a higher version.

Running python tests with pixi run py-test shows that a lot of tests are now broken because of the emitted warning. Haven't looked through the exact cause on all of them whether the warning should be handled, or strict mode is leaking etc. but this clearly needs some attention

[AhmedMousa-ag]

I found that returning the warning result to python caused multipile errors on the python side, I will ignore it as it was. I believe it's enough to see the warning on in terminal, logs and be known in python side. instead of raising as an error and hault the whole process.

[Wumpf]

I really don't want to create precedence in ignoring warnings. This means that strict_mode no longer works as expected everywhere.
I'm looking into fixing up those tests instead

[Wumpf]

yikes. Turns out we never actually delete rust sided recordings ever. I.e. this:

        with rr.RecordingStream("rerun_example_test_recording") as rec:
            pass

will leak a recording on the rust side 😱

Looking into this as well now separately

[Wumpf]

the leaking recordings is actually a known issue 😬

• Drop unused python recording streams #2116

tried to tackle it, but I need help from @abey79 for that. Wasn't too hard to get things to work out without. So this should do now if ci passes.

[AhmedMousa-ag]

Why not to delete the recording on __del__() destructor when the recording stream is deleted? Here

The implementation in rust and in python should be straight forward.

/// Delete a recording stream by id.
#[allow(clippy::fn_params_excessive_bools)]
#[allow(clippy::struct_excessive_bools)]
#[pyfunction]
#[pyo3(signature = (
    recording_id,
))]
fn delete_recording(_py: Python<'_>, recording_id: String) -> PyResult<()> {
    if all_recordings()
        .remove(&StoreId::from_string(
            StoreKind::Recording,
            recording_id.clone(),
        ))
        .is_none()
    {
        utils::py_rerun_warn(format!("Recording id: {recording_id} not found.").as_str())?;
    };
    Ok(())
}

the leaking recordings is actually a known issue 😬

• Drop unused python recording streams #2116

tried to tackle it, but I need help from @abey79 for that. Wasn't too hard to get things to work out without. So this should do now if ci passes.

[Wumpf]

I think that's what we going end up doing in some form or capactiy but I found untangling the ramifications rather complicated. See:

• rerun/rerun_py/src/python_bridge.rs

  Line 47 in ae4c7e2

  // The bridge needs to have complete control over the lifetimes of the individual recordings,

• rerun/rerun_py/src/python_bridge.rs

  Line 334 in ae4c7e2

  // NOTE: Do **NOT** try and drain() `all_recordings` here.

• rerun/rerun_py/src/python_bridge.rs

  Line 465 in ae4c7e2

  // Swapping the active data recording might drop the refcount of the currently active recording

Recording streams are internally already refcounted but we have this all_recordings registry on top and the Python object that wraps the recording is rather confusing as well: removing from the all_recordings registry doesn't do the actual drop which (see above comment links) is "dangerous" if done in the wrong spot, therefore the "real" drop of the inner recording stream (the one wrapped by the recording) happens at another point in time we don't control 😵‍💫
We might first need to make the python objects hold handles rather than "real" recording streams

[AhmedMousa-ag]

Ok, so we don't know what the grabage collector doing at any time, but can't we invoke the garabge collection in the destructor? gc.collect().
Usually invoking the garbage collection should be avoided, but is it worth it? will it solve the issue?

[Wumpf]

There's no gurarantees what-so-ever on what gc.collect exactly does, it's more of a hint