[CPP-80]Add realtime delay when reading an SBPfile and pip cache to GA. #43

john-michaelburke · 2021-04-28T02:42:32Z

This change currently points to a branch undergoing review:
~~https://github.com/swift-nav/thachoppa/pull/18~~ swift-nav/libsbp#967

Attempts introducing an artificial delay while reading SBP files to mimic a realtime viewing.
Adds pip caching to GA.

~~TODO (Want to get some feedback on the approach first)~~

~~fix current unit tests to accommodate this feature~~
~~adjust benchmarks using this new feature~~
~~add some unit testing to the realtime_delay function in maintab~~

…hachoppa integration.

console_backend/Cargo.toml

console_backend/src/process_messages.rs

…aelburke/realtime

.github/workflows/main.yml

…aelburke/realtime

john-michaelburke · 2021-04-29T22:11:24Z

The benchmarks in CI needed to be adjusted for this new delay. I choose not to use the delay for our backend cpu benchmark because the aim is to get worst case CPU usage so delay would counter this.

The Backend Mem benchmark greatly increased for Windows (Not too sure why) ~90MB mean +/- 40MB.

I added caching for all pip install stages in CI and it seems to moderately reduce build time with the exception of Windows, @jayvdb I think once you get the pycapnp wheel working, the reduction will be huge for windows in tandem with this change. Seems the other two wheels not being cached are fbs and pymany; however, I am not sure if they take much time at all.

I had to add LLVM/Clang to the CI to support the swiftnav-rs dependency through libsbp with this new GpsTime feature. This also adds a significant amount of time to the build sadly (Maybe it would be worth investigating caching Rust).

jayvdb · 2021-04-29T23:11:07Z

.github/workflows/main.yml

+      - name: Cache pip
+        uses: actions/cache@v2
+        with:
+          path: ~/Library/Caches/pip


rather than having three cache locations, set PIP_CACHE_DIR to a sensible directory. '~/.cache/pip` is a good choice. Understand you may be tired of CI; I can take this up in my next PR.

Done. Haha definitely a bit tired of CI but that's quite alright just getting better. Good suggestion this cleans up a lot of the repetition.

jayvdb · 2021-04-29T23:15:19Z

.github/workflows/main.yml

      - name: Build ${{ runner.os }} Binaries.
        env:
-          OS_NAME: ${{ runner.os }}
+          LIBCLANG_PATH: "C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/VC/Tools/Llvm/x64/bin"


use a top(workflow) level env for this, so it isnt repeated multiple times.

I believe this is different for each operating system. Is there a way to set it only for certain OSs?

Right, either use if on the env, if that is possible at the global level, or set LIBCLANG_PATH_WIN: .., and then use that global in the per OS jobs where it is needed.

Done. I ended up going with the LIBCLANG_PATH_WIN. It would be nice to investigate an if on the env later on though so we can combine the two steps again.

jayvdb · 2021-04-29T23:21:12Z

console_backend/src/process_messages.rs


            _ => {
-                // no-op
+                continue;


is intended to avoid delays for messages we dont know about?

or avoid log flushes?

Either way, the reason should be added as a comment here.

I am doing it to prevent unrelated messages from forcing a delay. Not really sure what the best approach is as I know it prevents from log flushing. I could alternatively set a boolean here and check it on each iteration? What do you think?

set a boolean, and pass it to realtime_delay, so that it can log when a delay was encountered and ignored.

I added a boolean but don't pass it to the realtime_delay. I decided not to go with the logging. I believe we would be getting something on the order of 5-10+ messages (not consumed) every 100ms so I think as small of a footprint as possible is ideal.

IMO we should still log them, but only if there is a delay in the message, or it is a message type with delay that we know is not appropriate when trying to simulate realtime delays ; they can be logged at the level below what is normally shown to the user. Currently anything below log::LevelFilter::Info is omitted, so Debug level would do the trick for now.
Our logger can also become smarter to collapse duplicated or similar messages.

I can work on this in my next log panel PR - I actually want a lot of logging activity, so it is easier to see how the log panel will perform if the console is left open for days.

Alright I added a debug for skipped messages and when a sleep occurs. How could I test out a different log level locally?

john-michaelburke · 2021-04-30T16:57:04Z

@silverjam @notoriaga PTAL

notoriaga · 2021-04-30T17:15:30Z

console_backend/Cargo.toml

 csv = "1"
 paste = "1"
 pyo3 = { version = "0.13", features = ["extension-module"] }
-sbp = { git = "https://github.com/swift-nav/libsbp.git", rev = "0563350bb387e4a966a2b558adb7b05b2baf005f" }


Would it be much effort to switch back to the choppa here? Not sure when this will be done. I have some changes that I'm gonna push soon which will break things (but I can just make a new branch)

When it's all set I can go and switch this + the choppa itself to use the new libsbp stuff

I think for the sake of the demo Monday it would be great if we could live with this. I wonder if it is possible to target a revision of a branch in cargo toml? Maybe that could allow you to continue developing.

Ahh don't worry about it, I'll just leave that one be and push a new one, probably will be easier to look at that way as well

@notoriaga Too late. I figured it out. I guess you just provide the revision and drop the branch name and everything is groovy!

I guess you just provide the revision and drop the branch name and everything is groovy!

Nice, good to know

silverjam

one nit, but otherwise lgtm

silverjam · 2021-04-30T22:50:00Z

console_backend/src/types.rs

                let shared_state_clone_ = shared_state.clone();
                let messages = sbp::iter_messages(stream);
-                process_messages(messages, shared_state_clone_, client_send.clone());
+                process_messages(messages, shared_state_clone_, client_send.clone(), true);


Bare bools are not descriptive at the call site, I would prefer an enum, or simply:

Suggested change

process_messages(messages, shared_state_clone_, client_send.clone(), true);

process_messages(messages, shared_state_clone_, client_send.clone(), /* realtime_delay = */ true);

Done. I created an Enum for the bare boolean in process_messages corresponding to RealtimeDelay::On/Off, the other bare delay was in the connect_to_file for whether or not to close after finishing where I only added an inline comment as you suggest.

john-michaelburke added 2 commits April 27, 2021 17:34

Have preliminary realtime delay working need to figure plan out for t…

fe12bed

…hachoppa integration.

Working now.

a66b132

john-michaelburke requested review from jayvdb, notoriaga and silverjam April 28, 2021 02:42

silverjam reviewed Apr 28, 2021

View reviewed changes

console_backend/Cargo.toml Outdated Show resolved Hide resolved

respond to review requests.'

ea0bd9a

jayvdb reviewed Apr 28, 2021

View reviewed changes

console_backend/Cargo.toml Outdated Show resolved Hide resolved

jayvdb reviewed Apr 28, 2021

View reviewed changes

console_backend/Cargo.toml Outdated Show resolved Hide resolved

console_backend/src/process_messages.rs Outdated Show resolved Hide resolved

john-michaelburke added 5 commits April 28, 2021 12:31

Swapped out thachoppa for sbp.

325910e

working in week.

fc798ab

Unittests and adjusting benches.

72be7c1

Merge branch 'main' of github.com:swift-nav/console_pp into john-mich…

183a2b6

…aelburke/realtime

Cleaning up merge.

02aae0e

john-michaelburke force-pushed the john-michaelburke/realtime branch from 8544357 to 27a42eb Compare April 29, 2021 02:19

Add clang/llvm to windows GA.

ca83fca

john-michaelburke force-pushed the john-michaelburke/realtime branch from 27a42eb to ca83fca Compare April 29, 2021 02:48

jayvdb reviewed Apr 29, 2021

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

john-michaelburke force-pushed the john-michaelburke/realtime branch 2 times, most recently from 05767e9 to 40b95ab Compare April 29, 2021 03:28

Merge branch 'main' of github.com:swift-nav/console_pp into john-mich…

35b4e62

…aelburke/realtime

john-michaelburke force-pushed the john-michaelburke/realtime branch from 40b95ab to 35b4e62 Compare April 29, 2021 03:30

Added smaller sbp file.

68e1868

john-michaelburke force-pushed the john-michaelburke/realtime branch 7 times, most recently from 42b1806 to 07dfbdb Compare April 29, 2021 06:35

john-michaelburke force-pushed the john-michaelburke/realtime branch 8 times, most recently from a8182ea to cabdeea Compare April 29, 2021 21:20

Merge branch 'main' of github.com:swift-nav/console_pp into john-mich…

ac66a2c

…aelburke/realtime

john-michaelburke force-pushed the john-michaelburke/realtime branch from cabdeea to ac66a2c Compare April 29, 2021 21:38

john-michaelburke requested review from jayvdb and silverjam April 29, 2021 22:05

jayvdb reviewed Apr 29, 2021

View reviewed changes

john-michaelburke force-pushed the john-michaelburke/realtime branch from dc2b64a to f7bf1f4 Compare April 30, 2021 02:05

Respond to review requests.

18039a9

john-michaelburke force-pushed the john-michaelburke/realtime branch from f7bf1f4 to 18039a9 Compare April 30, 2021 02:14

jayvdb approved these changes Apr 30, 2021

View reviewed changes

Respond to review requests.

382d120

notoriaga reviewed Apr 30, 2021

View reviewed changes

Lock revision of libsbp.

a54e09b

john-michaelburke changed the title ~~[CPP-80]Add realtime delay when reading an SBPfile.~~ [CPP-80]Add realtime delay when reading an SBPfile and pip cache to GA. Apr 30, 2021

silverjam approved these changes Apr 30, 2021

View reviewed changes

Respond to review requests.

1d57974

silverjam approved these changes Apr 30, 2021

View reviewed changes

john-michaelburke merged commit 19472b9 into main Apr 30, 2021

john-michaelburke deleted the john-michaelburke/realtime branch April 30, 2021 23:58

	process_messages(messages, shared_state_clone_, client_send.clone(), true);
	process_messages(messages, shared_state_clone_, client_send.clone(), /* realtime_delay = */ true);

[CPP-80]Add realtime delay when reading an SBPfile and pip cache to GA. #43

[CPP-80]Add realtime delay when reading an SBPfile and pip cache to GA. #43

Uh oh!

Conversation

john-michaelburke commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

john-michaelburke commented Apr 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-michaelburke commented Apr 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverjam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

john-michaelburke commented Apr 28, 2021 •

edited

Loading