ForwardMsg de-duping by tconkling · Pull Request #28 · streamlit/streamlit

tconkling · 2019-08-27T21:34:04Z

This is the first part of ForwardMsg caching:

Server

ForwardMsg.hash is calculated on every outgoing ForwardMsg
The server caches all messages whose size is >= 10k in its MessageCache instance. (These cached message are never evicted - this will come in a future PR.) Each entry in the cache contains the cached message, and also a (weak-referenced) set of all ReportSessions for which that message has already been delivered.
Cached ForwardMsgs have the metadata.cacheable flag set. When the client receives a ForwardMsg with cacheable == true, it also caches it.
If the server believes that a client has cached a given ForwardMsg (because the cache has an entry for that message+session), it instead sends a "reference" message that points to the original message via ForwardMsg.ref_hash.
The server also exposes a new HTTP endpoint, /message, from which clients can request messages that they have received a ref_hash message for, but are missing in that client's local cache. (This should not happen frequently! If clients are getting a bunch of local cache misses, then the server is not maintaining its cache properly.)

Client

WebsocketConnection has a new ForwardMsgCache member
All incoming ForwardMsgs are passed through this cache (via ForwardMsgCache.processMessage):
- If the ForwardMsg is not a reference message, it is cached locally and nothing more is done
- If the ForwardMsg is a reference message, the client checks to see if it has the original message in its cache; if so, it gets returned
- If the reference message is missing from the client's cache, it is requested from the server's /message endpoint, and then cached.

TODO:

The client and server both need to perform cache eviction; currently, their caches never have anything removed from them.
I'd like to have some better client tests that use a mock server to test its handling of the /message endpoint

It doesn't make sense to try construct a new Server instance here, especially when we're not passing any of its required args.

* tim/ServerRoutingRefactor: imports DebugHandlerTest stub MetricsHandlerTest HealthHandler & test Remove server param from MetricsHandler streamlit.server package, containing Server and routes Server.get_current: raise if the server doesn't exist

* tim/ServerRoutingRefactor: server_util functions

* develop: Server route tests (streamlit#1220) Improving image processing times (#1247) Support hashing `functools.partial` (streamlit#1242) Graphviz chart cypress tests (streamlit#1215) Fix bug where sliders were always returning a tuple (streamlit#1243) st.image() format parameter (streamlit#1239) Cypress tests for empty charts (streamlit#1232) Ignore VSCode configs (streamlit#1241) Add font smoothing antialiased to html body in cypress tests (streamlit#1233) Write instance of list as json (#1228) streamlit#1159 add parameter to specify "BGR" color channel in st.image (streamlit#1222) streamlit#1178 Tweaking st.slider errors (streamlit#1208) Send the user's Python version to Segment (streamlit#1212) Clean up our protos (streamlit#1211) Fix a single word in our docs and lint some files ;) (streamlit#1229) Fix DPR issues with Cypress snapshots (streamlit#1213)

This means that tests.streamlit is a proper package (so modules can import from each other). It also means that several tests that check qualified module names needed to be updated to reflect their new paths.

* tim/TestableServer: Argh. Fix Python 2 testing Add missing __init__.py to tests.streamlit ServerTestCase base class Server_test.test_websocket_connect Server.start function

* develop: Testable server (streamlit#1257) DataFrames bigger than 2 dimensions will be shown as text (streamlit#1255) Circleci Insights (streamlit#1254) Setting listeners on XMLHttpRequest upfront (streamlit#1251) Matplotlib fix (streamlit#1253) Warn about mutations of arguments of cached functions (streamlit#1249) Hash numpy arrays and sample large dataframes (streamlit#1252) Improve function context, ignore streamlit functions (streamlit#1240) Cypress tests for table styling Setup cypress dashboard Showing namedtuples as JSON (streamlit#1248) Make the "Is Streamlit still running?" dialog's message nicer (streamlit#1231) Move caching test, fix function names (streamlit#1250)

* develop: Fix streamlit#1230: OSError not caught when writing cache to disk (streamlit#8) Improve caching code (streamlit#2) Remove incorrect background of code inside error messages (streamlit#4) Fix line number in error message (streamlit#3) Update README.md Implement Caching object to support running code blocks only once (streamlit#1) Update README.md Add font licenses to NOTICE (streamlit#1267) Clean up repo for open source release. Remove unecessary folders. (streamlit#1266) Fix st.cache (streamlit#1273) Close streamlit#1092 (streamlit#1270) Change license header in variables.scss so it's compatible with scss-to-json. (streamlit#1268) Sidebar (streamlit#1224) Add Apache license and update all license headers (streamlit#1265) Handling objects with empty docs in st.help (streamlit#1261) Enhance Mochawesome report with screenshots (streamlit#1263) Add LICENSES file (streamlit#1262)

* develop: ForwardMsg.metadata (streamlit#21) Remove hexagon layer from st.map, use Pandas for range computation, and other st.map improvements (streamlit#20)

tconkling · 2019-08-27T21:39:18Z

frontend/src/lib/WebsocketConnection.tsx

+    // Dispatch any pending messages in the queue. This may *not* result
+    // in our just-decoded message being dispatched: if there are other
+    // messages that were received earlier than this one but are being
+    // downloaded, our message won't be sent until they're done.


This chunk of code below isn't any different from before; I just added the explanatory comment because I had a hard time figuring out what it did.

This is delivered to the client in the Initialize message

tvst

Did a partial review. Only looked at frontend code so far.

frontend/src/lib/ForwardMessageCache.ts

tvst · 2019-08-28T07:09:05Z

frontend/src/lib/ForwardMessageCache.ts

+   * - If the referenced message isn't in our cache, request it from the
+   *   server, cache it, and return it.
+   */
+  public async processMessage(msg: ForwardMsg): Promise<ForwardMsg> {


How about naming this something like getFullMessage, maybeFillPayload, or fillPayloadIfNeeded?

Are you cool with processMessagePayload? The method is doing more than simply filling the message payload: it is also caching that payload, when appropriate; the other suggestions imply to me that the function doesn't mutate the cache's own internal state, but it does.

frontend/src/lib/ForwardMessageCache.ts

frontend/src/lib/ServerUtil.ts

tvst · 2019-08-28T07:24:42Z

frontend/src/lib/ServerUtil.ts

+/**
+ * Creates a ws:// or wss:// URI for the given path.
+ */
+export function buildWsUri({host, port}: BaseUriParts, path: string): string {


We only have 1 websocket URI, so no need to path. Should always be 'stream'

It's not a huge deal, but: why would we hardcode this path into a generic utility function? It seems like the hardcoded path, which is "business logic", should remain in WebsocketConnection - possibly being promoted to a const at the top of that file. This would also mean that buildWsUri and buildHttpUri continue to have consistent signatures.

Fine :)

I tend to prefer to make functions only as generic as they have to be. But in this case it's not a big deal.

possibly being promoted to a const at the top of that file.

Can you do this, at least?

tvst · 2019-08-28T07:30:02Z

frontend/src/lib/WebsocketConnection.tsx

+          // TODO: do something reasonable here, beyond simply logging.
+          //  Lots of stuff is likely to be broken!
+          logError(LOG, reason)
+        })


You could call this.stepFsm('CONNECTION_ERROR')

For extra credit, you can pipe the reason all the way to ConnectionManager.setConnectionState (via WebsocketConnection.args.onConnectionStateChange)

This is what I tried first (sending the CONNECTION_ERROR event), but WebsocketConnection doesn't handle that properly (it fails with an assertion shortly thereafter, when it starts trying to ping the server).

I can fix this, though since we don't have tests set up for any of this websocket stuff, it's dicey; there's at least several bits of state are not being properly cleared during certain state transitions. (The bit I was running into here is the class expecting its this.websocket to be null/undefined when entering the PINGING_SERVER state, which the CONNECTION_ERROR event handler does not do.)

Ahhhh... I see.

Ok, leave it as is for now 👍

tvst · 2019-08-28T17:07:37Z

frontend/src/lib/ServerUtil.ts

+/**
+ * Creates a ws:// or wss:// URI for the given path.
+ */
+export function buildWsUri({host, port}: BaseUriParts, path: string): string {


Fine :)

I tend to prefer to make functions only as generic as they have to be. But in this case it's not a big deal.

possibly being promoted to a const at the top of that file.

Can you do this, at least?

frontend/src/lib/ForwardMessageCache.ts

tvst · 2019-08-28T17:10:51Z

frontend/src/lib/WebsocketConnection.tsx

+          // TODO: do something reasonable here, beyond simply logging.
+          //  Lots of stuff is likely to be broken!
+          logError(LOG, reason)
+        })


Ahhhh... I see.

Ok, leave it as is for now 👍

tvst · 2019-08-28T17:13:03Z

frontend/src/lib/WebsocketConnection.tsx

    if (this.websocket) {
      this.websocket.close()
-      this.websocket = null
+      this.websocket = undefined


Why switch this?

I tend to use null for anything that is set manually. This way we can easily tell from the type whether we manually caused something to have no value (null) or if it was initialized that way (undefined).

In normal JS this helps uncover some types of bugs, although in TS it may not make a difference.

I changed the field's type from Websocket? | null to Websocket?, so null is no longer a value that can be assigned to the field.

I can see the undefined-vs-null distinction for JS, but if a field is declared as an optional in Typescript, it's not idiomatic to also assign null to it.

lib/streamlit/MessageCache.py

lib/streamlit/server/routes.py

tvst · 2019-08-28T17:36:44Z

lib/streamlit/server/routes.py

+        """
+        self._cache = cache
+
+    def set_default_headers(self):


Why not subclass _SpecialRequestHandler and remove these methods?

(In which case you should rename _SpecialRequestHandler to something like _HttpRequestHandler)

_SpecialRequestHandler sets the no-cache header, which I think we explicitly don't want for these cacheable messages

lib/streamlit/server/routes.py

tconkling added 30 commits August 14, 2019 11:15

Begin on MessageCache

5deee34

Server.get_current: raise if the server doesn't exist

8798b5d

It doesn't make sense to try construct a new Server instance here, especially when we're not passing any of its required args.

streamlit.server package, containing Server and routes

c0f9f5b

Remove server param from MetricsHandler

64a39f4

HealthHandler & test

92e4e9c

MetricsHandlerTest

ab78e57

DebugHandlerTest stub

af43575

imports

ec70f81

server_util functions

6fe95e4

Merge branch 'tim/ServerRoutingRefactor' into tim/ForwardMsgDeDuping

9294f07

* tim/ServerRoutingRefactor: server_util functions

MessageCacheHandler route

6b3314d

server_util.should_cache_msg

1418744

MessageCache: create_reference_msg

f29a8e7

Server._send_message (WIP)

4266b2e

Fixup protobuf imports

d143f0b

Server.start function

983c4a0

Server_test.test_websocket_connect

ac74461

ServerTestCase base class

8eb998a

Add missing __init__.py to tests.streamlit

80aa0a1

This means that tests.streamlit is a proper package (so modules can import from each other). It also means that several tests that check qualified module names needed to be updated to reflect their new paths.

Argh. Fix Python 2 testing

85479d8

Merge branch 'tim/TestableServer' into tim/ForwardMsgDeDuping

f2661fb

* tim/TestableServer: Argh. Fix Python 2 testing Add missing __init__.py to tests.streamlit ServerTestCase base class Server_test.test_websocket_connect Server.start function

ServerTest.test_duplicate_forwardmsg_caching

56eb91b

WebsocketConnection.handleMessage: async-ify, add a comment

8b369dd

ForwardMessageCache

f1a57b6

examples/core/message_deduping.py

797e9ad

ForwardMsg.id -> ForwardMsg.hash

ea0b733

tconkling added 6 commits August 27, 2019 13:37

Merge branch 'develop' into tim/ForwardMsgDeDuping

4535ab2

* develop: ForwardMsg.metadata (streamlit#21) Remove hexagon layer from st.map, use Pandas for range computation, and other st.map improvements (streamlit#20)

Cache our incoming messages on the client

ce1a5a3

Simplify

5b182a7

Throw better errors

bf48d80

Formatting

1e9e692

MessageCacheTest.test_has_message

42b2072

tconkling requested a review from tvst August 27, 2019 21:34

tconkling commented Aug 27, 2019

View reviewed changes

tconkling added 6 commits August 27, 2019 14:47

Slightly clearer

ed1bc5f

Comment fix

8b23f1e

Whoops - test_has_message is redundant

63a337e

Whoops, fix a couple constants

f3184e7

SessionInfo: commandLine is not an Arg

a73126b

New config: global.minCachedMessageSize

c5f58f6

This is delivered to the client in the Initialize message

tconkling mentioned this pull request Aug 27, 2019

Config: global.minCachedMessageSize #31

Closed

Remove stray semicolon

850d6ec

tvst reviewed Aug 28, 2019

View reviewed changes

tconkling added 7 commits August 28, 2019 09:52

MetricsManager test: fix a couple type issues

4d5280b

server_util.should_cache_msg -> is_cacheable_msg

210a06e

ForwardMsg.metadata.cacheable is true if a message should be cached

98a59d8

ForwardMsgCache.processMessage -> processMessagePayload

c50426a

ForwardMessageCache.fetchMessagePayload

2341828

Header fix

0b316d3

ServerUtil -> UriUtil

c208322

tvst approved these changes Aug 28, 2019

View reviewed changes

tconkling added 3 commits August 28, 2019 11:24

Addressing feedback

851db54

CORS /OPTIONS docs

6da1a3b

WEBSOCKET_STREAM_PATH

f81f67b

tconkling merged commit 10061f7 into streamlit:feature/hashing Aug 28, 2019

tconkling deleted the tim/ForwardMsgDeDuping branch August 28, 2019 20:35

Conversation

tconkling commented Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Server

Client

TODO:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tconkling commented Aug 27, 2019 •

edited

Loading