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

Defaulting the RecordingId to authkey() should be opt-in #3793

Open
nikolausWest opened this issue Oct 11, 2023 · 5 comments
Open

Defaulting the RecordingId to authkey() should be opt-in #3793

nikolausWest opened this issue Oct 11, 2023 · 5 comments
Labels
🪳 bug Something isn't working 🐍 Python API Python logging API
Milestone

Comments

@nikolausWest
Copy link
Member

nikolausWest commented Oct 11, 2023

UPDATE:


Describe the bug
When I create two recordings with the same application id using the python SDK, only a single recording is shown in the viewer.

To Reproduce

import rerun as rr
rr.init("test", spawn=True)
rr.init("test", spawn=True)

Expected behavior
The viewer should show two recordings with the application id "test"

Screenshots
It only shows a single recording
Screenshot 2023-10-11 at 10 30 40

Rerun version
0.9

@nikolausWest nikolausWest added 🪳 bug Something isn't working 🐍 Python API Python logging API labels Oct 11, 2023
@nikolausWest nikolausWest added this to the 0.9.1 milestone Oct 11, 2023
@nikolausWest
Copy link
Member Author

I put this on the 0.9.1 milestone but it's ok to move if it's too hard to get in in time.

@teh-cmc teh-cmc self-assigned this Oct 11, 2023
@teh-cmc
Copy link
Member

teh-cmc commented Oct 11, 2023

This works:

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let rec =
        rerun::RecordingStreamBuilder::new("test").connect(rerun::default_server_addr(), None)?;
    let rec =
        rerun::RecordingStreamBuilder::new("test").connect(rerun::default_server_addr(), None)?;
    Ok(())
}

This works:

int main() {
    {
        auto rec = rr::RecordingStream("test");
        rec.connect("127.0.0.1:9876").throw_on_failure();
    }

    {
        auto rec = rr::RecordingStream("test");
        rec.connect("127.0.0.1:9876").throw_on_failure();
    }
}

This doesn't work:

rr.init("test")
rr.connect()
rr.init("test")
rr.connect()

This doesn't work:

rr.init("test", spawn=True)
rr.init("test", spawn=True)

This doesn't work:

rr.new_recording(application_id="test", make_default=True, spawn=True)
rr.new_recording(application_id="test", make_default=True, spawn=True)

This doesn't work:

rec = rr.new_recording(application_id="test", make_default=True)
rec.connect()
rec = rr.new_recording(application_id="test", make_default=True)
rec.connect()

This works:

rec = rr.new_recording(application_id="test1", make_default=True, spawn=True)
rec = rr.new_recording(application_id="test2", make_default=True, spawn=True)

And most importantly, this works:

rec = rr.new_recording(application_id="test", recording_id="a", make_default=True, spawn=True)
rec = rr.new_recording(application_id="test", recording_id="b", make_default=True, spawn=True)

@teh-cmc
Copy link
Member

teh-cmc commented Oct 11, 2023

This actually behaves as expected, it's just surprising that the recording ID defaults to the process' authkey: this should be opt-in.

Decision:

@teh-cmc teh-cmc changed the title Creating new recordings in the Python SDK doesn't result in new recordings in the viewer Defaulting the RecordingId to authkey() should be opt-in Oct 11, 2023
@teh-cmc teh-cmc removed their assignment Oct 11, 2023
@emilk
Copy link
Member

emilk commented Oct 24, 2023

I suggest we do this at the same time as we refactor init overall

@jleibs
Copy link
Member

jleibs commented Nov 28, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 🐍 Python API Python logging API
Projects
None yet
Development

No branches or pull requests

4 participants