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

run_all.py: add --fast, --separate, and --close #2054

Merged
merged 16 commits into from
May 10, 2023

Conversation

jprochazk
Copy link
Member

@jprochazk jprochazk commented May 5, 2023

Fixes #1945

  • Refactored the run_all.py script
  • Added --fast flag which limits examples to just cherry-picked examples
  • Added --separate flag which runs each example in a separate viewer
    • For --web, this opens a new tab for each example
    • For native/--save, this opens a new window for each example
  • Added --close flag which closes all open viewers when the examples finish running

The refactoring mainly consisted of moving the viewer starting logic into a class which can be used as a context manager in a with statement. It also finds and uses free ports every time it's started, which makes running it much more consistent (no more port already in use warnings).

Checklist

PR Build Summary: https://build.rerun.io/pr/2054

@jprochazk jprochazk added the 🧑‍💻 dev experience developer experience (excluding CI) label May 5, 2023
@jprochazk jprochazk force-pushed the jan/run-separate-fast-examples branch from 3f238cb to 612bcb5 Compare May 5, 2023 11:22
@jprochazk
Copy link
Member Author

jprochazk commented May 5, 2023

A few usage examples:

# Open fast examples, each in their own web viewer tab
$ ./scripts/run_all.py --fast --separate --web

# Open all examples sequentially in the same viewer
$ ./scripts/run_all.py

# Skip building the SDK, open fast examples, save them to .rrd files, then load each one in a separate window
$ ./scripts/run_all.py --skip-build --fast --save --load --separate

# Skip building the SDK, open fast examples sequentially, loaded from `out.rrd` files
$ ./scripts/run_all.py --skip-build --fast --load

@jprochazk jprochazk requested a review from emilk May 5, 2023 15:34
@emilk
Copy link
Member

emilk commented May 8, 2023

When I run ./scripts/run_all.py --fast --separate I get at least one window with

image

Running ./scripts/run_all.py --fast --separate --web gets me several tabs with the same message.

I haven't reviewed the code yet, but seems to me there is a place or two where we need to add a wait.

For instance, we need need to get the web browser time to connect to the rerun server (and download the log data) before shutting it down. The easy way here is to give it 5-10 seconds or something. The robust solution is quite a bit more involved, as it involves figuring out if the rerun process has both received the full log stream and also served it on in re_ws_comms::RerunServer. I suggest we punt on that.

Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Really nice, but there are some weirdness, at least on my Mac!

I would also love for the text output of run_all.py to be improved. Especially when running in sequential mode it would be good if each example was clearly delineated and marked, e.g. with a blank line and a `print("\nRunning {args}…")

print(f"process exited with error code {returncode}")
if wait:
returncode = process.wait()
print(f"process exited with error code {returncode}")
Copy link
Member

Choose a reason for hiding this comment

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

Any non-zero returncode should result in run_all.py returning non-zero, and printing a big fat error message

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this to assert instead. Not sure how I should do the same when running --separate.

scripts/run_all.py Outdated Show resolved Hide resolved
scripts/run_all.py Outdated Show resolved Hide resolved
scripts/run_all.py Outdated Show resolved Hide resolved
scripts/run_all.py Outdated Show resolved Hide resolved
scripts/run_all.py Outdated Show resolved Hide resolved
scripts/run_all.py Outdated Show resolved Hide resolved
scripts/run_all.py Outdated Show resolved Hide resolved
scripts/run_all.py Outdated Show resolved Hide resolved
scripts/run_all.py Outdated Show resolved Hide resolved
@jprochazk jprochazk force-pushed the jan/run-separate-fast-examples branch from 69e768c to 71f7909 Compare May 8, 2023 09:55
@jprochazk jprochazk force-pushed the jan/run-separate-fast-examples branch from e0737a8 to d159c7a Compare May 9, 2023 06:11
@jprochazk jprochazk requested a review from emilk May 9, 2023 06:12
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Works great! ⭐

@jprochazk jprochazk merged commit ca5aa68 into main May 10, 2023
@jprochazk jprochazk deleted the jan/run-separate-fast-examples branch May 10, 2023 05:24
jprochazk added a commit that referenced this pull request May 11, 2023
* add `--fast`, `--separate`, and `--close`

* split `--save` into `--save` + `--load`

* minor refactoring

* assert that return code is non-zero when waiting

* cast to int directly

* reverse condition for `fast` in `collect_examples`

* sort cherry-picked examples by name

* rename `port` to `sdk_port`

* add descriptions for ports

* use built `rerun` binary directly + simplify args

* wait for viewers to start + examples to finish

* dont start viewer for `--save`

* add `--quiet` to build commands

* sleep instead of checking logs

* capture output and print at the end

* print what we are waiting for

---------

Co-authored-by: Emil Ernerfeldt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑‍💻 dev experience developer experience (excluding CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

run_all_examples.py helper script
2 participants