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

When launched with a file path, the viewer will now still listen for TCP connections #6951

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

petertheprocess
Copy link

@petertheprocess petertheprocess commented Jul 20, 2024

What

related issue #6952
When lauched with files rerun xxx.rrd, the viewr doesn't listen to any port.
I am using rerun-cli 0.17.0 [rustc 1.76.0 (07dca489a 2024-02-04), LLVM 17.0.6] x86_64-unknown-linux-gnu release-0.17.0 d65ca50, built 2024-07-08T13:47:55Z.

Before this PR:
No log msg shows we are hosting a SDK sever over some ports.

> rerun /media/Q/eulerBackUp/Box/Box/rerun_samples.rrd
[2024-07-20T19:37:04Z INFO  re_data_loader::load_file] Loading "/media/Q/eulerBackUp/Box/
Box/rerun_samples.rrd"…
[2024-07-20T19:37:04Z INFO  winit::platform_impl::platform::x11::window] Guessed window s
cale factor: 1
[2024-07-20T19:37:04Z WARN  wgpu_hal::vulkan::instance] Unable to find extension: VK_EXT_
swapchain_colorspace
[2024-07-20T19:37:04Z INFO  egui_wgpu] There were 2 available wgpu adapters: {backend: Vu

> rerun
[2024-07-20T19:38:40Z INFO  re_sdk_comms::server] Hosting a SDK server over TCP at 0.0.0.0:9876. Connect with the Rerun logging SDK.
[2024-07-20T19:38:40Z INFO  winit::platform_impl::platform::x11::window] Guessed window scale factor: 1
[2024-07-20T19:38:40Z WARN  wgpu_hal::vulkan::instance] Unable to find extension: VK_EXT_swapchain_colorspace

After this PR:

> target/debug/rerun /media/Q/eulerBackUp/Box/Box/rerun_samples.
[2024-07-20T19:43:09Z INFO  re_data_loader::load_file] Loading "/media/Q/eulerBackUp/Box/
Box/rerun_samples.rrd"…
[2024-07-20T19:43:09Z INFO  re_sdk_comms::server] Hosting a SDK server over TCP at 0.0.0.
0:9876. Connect with the Rerun logging SDK.
[2024-07-20T19:43:09Z INFO  winit::platform_impl::platform::x11::window] Guessed window scale factor: 1

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

@Wumpf Wumpf self-requested a review July 22, 2024 07:16
@Wumpf Wumpf changed the title Fix: When launched with a file path, the viewer doesn't listen to any TCP ports When launched with a file path, the viewer will now still listen on the default TCP port Jul 22, 2024
@Wumpf Wumpf changed the title When launched with a file path, the viewer will now still listen on the default TCP port When launched with a file path, the viewer will now still listen for TCP connections Jul 22, 2024
@Wumpf Wumpf added 📺 re_viewer affects re_viewer itself include in changelog labels Jul 22, 2024
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! Don't see any reason why we wouldn't listen on tcp after loading a file either.

@Wumpf
Copy link
Member

Wumpf commented Jul 22, 2024

python wheel build is a known failure. waiting for rust lint ci to finish, then merge

Copy link
Member

@teh-cmc teh-cmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break the ability to open multiple files in parallel.

Today, you can do e.g. this:

$ rerun file1.rrd  # in terminal 1
$ rerun file2.rrd  # in terminal 2

This will open two Rerun windows, one for each terminal.

This works fine because these two windows don't bind to any port, and so they cannot conflict with one another.

With this PR, this would now fail:

$ rerun file1.rrd  # in terminal 1
$ rerun file2.rrd  # in terminal 2 -- ERROR: port 9876 taken

You'd have to do this instead, which is really weird from a UX standpoint:

$ rerun file1.rrd              # in terminal 1
$ rerun file2.rrd --port 9877  # in terminal 2

Automatically switching the second viewer to another port that's available isn't really a solution either: it's very impractical to have a bunch of Rerun Viewers automatically open on different random ports.

I believe the correct solution is for rerun file.rrd to redirect the data to the already listening viewer for that port, if there is one.
To force-open a new viewer, the user can use --port XXXX as before (and if that port is also already taken, then the forwarding logic applies once again).

Related:

@teh-cmc
Copy link
Member

teh-cmc commented Jul 22, 2024

(Added details)

@petertheprocess
Copy link
Author

petertheprocess commented Jul 22, 2024

I believe the correct solution is for rerun file.rrd to redirect the data to the already listening viewer for that port, if there is one.
To force-open a new viewer, the user can use --port XXXX as before (and if that port is also already taken, then the forwarding logic applies once again).

This make sense, I can work on it. But I can't do that till Thursdy.


Here comes another question: how to detect a exist viewer and the corresponding Port?
I am not sure does the egui framework have some support for that?

Or we can create a temp file which logs the existing viwer's status, every time launch a new instance, we check this file.
@teh-cmc @Wumpf any idea?

@petertheprocess
Copy link
Author

I found that currentlly rerun_sdk handle multiple spawn by simplly assuming the port is running a Rerun Viewer.
[2024-07-23T21:11:25Z INFO re_sdk::spawn] A process is already listening at this address. Assuming it's a Rerun Viewer. addr=0.0.0.0:9876

@teh-cmc
Copy link
Member

teh-cmc commented Jul 24, 2024

Yes, for now you should just assume that if a port is taken, Rerun is what's bound to it.

In the future we will need to make this more robust, see:

( ⚠️ make sure to make a new message if you want to ask us a question -- if you do an edit, we do not get a notification, even if you tag us! 😱)

@petertheprocess
Copy link
Author

petertheprocess commented Sep 4, 2024

Sorry for the late reaction,
Now I have added the port checking logic for cli as #4019 did. But I cannot find a way to stream the file to current active recording.

....
   } else if is_another_viewer_running {
        re_log::info!("Another viewer is already running, streaming data to it.");
        // get current active recording
        use re_sdk::RecordingStream;
        let static_ = true;

        let Some(active_recording) = RecordingStream::get(re_sdk::StoreKind::Recording, None)
        else {
            anyhow::bail!("No active recording found!");
        };
        // map is lazy, so we need take the result by let _ = ...
        let _ = args
            .url_or_paths
            .iter()
            .map(|path| active_recording.log_file_from_path(path, None, static_));
        Ok(())
    } else {
...

I am tring to use re_sdk's log_file_from_path to do that, but I can't get access to the recording in a running viewr. In python api, it seems like a new recording will be created if a viewr is running. But I think the best default behavior should be streamming to the active record so that we can megre 2 .rrd file or append new log to a existing one. Any idea?

@petertheprocess
Copy link
Author

@Wumpf @teh-cmc

@Wumpf
Copy link
Member

Wumpf commented Sep 9, 2024

But I think the best default behavior should be streamming to the active record so that we can megre 2 .rrd file or append new log to a existing one. Any idea?

Sure that would be a nice variant, but that's not what passing files in does today either. E.g. when passing in two images to rerun cli, it creates two application ids one for each image.
Which essentially means that there's two recording streams. There's no real concept of querying a RecordingStream since a recording stream is just a utility to send data to a server. If you'd really want to stream to the same recording, you'd use the same application id and recording id and things would merge upon arrival at the server. But again, I don't think that's what we want here anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When launched with a file path, the viewer doesn't listen to any TCP ports.
3 participants