chore: introduce and use observables with improved update checking #8405

ashtonG · 2023-11-13T21:51:56Z

Description

this introduces two patterns that make it easier for developers to work with observable values without introducing extra renders and events:

deepObservable and immutableObservable: These observables do an
extra check before setting the observable value. deepObservable does a
deep equality check, and immutableObservable uses Immutable.is. If
the old and new values are the same via this check, we throw out the new
value.
asValueObject: the immutableObservable type from
above by itself isn't enough to solve the problem for our usage of
immutables, because we use plain javascript objects and arrays inside
immutable objects. By default, immutable uses reference equality to
determine if two immutable collections are the same, e.g.
```
import { is, Map } from 'immutable'

const map = Map({ 'hi': 'hello' })
is(map.set('hi', 'hello'), map) // true

const newMap = map.set('bye', {})
is(newMap.set('bye', {}), newMap) // false
```
however, if an object implements the functions hashCode: () => number
and equals: (v: unknown) => boolean, Immutable.is will use those
functions to determine whether or not two objects are the same. To
facilitate doing this, we introduce asValueObject. asValueObject
takes a codec and a object implementing that codec and returns a version
of the value implementing ValueObject. The resulting value object uses
the codec to determine which values on the object to check against and
how to hash the object. Checking happens as deep as the codec goes,
falling back to value equality, so props that aren't in the codec aren't
checked and props that have indeterminate types (like any, unknown,
or never) will use === to check equality.

Test Plan

as an admin, visit the cluster overview page:

the cluster overview stats should show up
the "active experiments" count should be accurate
the "active jupyterlabs/tensorboards/shells/commands" count should be accurate
the "connected agents" count should be accurate

as an admin, visit the user management page:

the user list should show up
you shouldn't be able to select yourself in the user list

when viewing the workspace list:

all workspaces the user has access to should be visible

Commentary (optional)

A minor caveat with asValueObject is that it works via spreading the input object -- as such, it requires the input codec to be an object type or interface, not an array. This means that observables that use immutable types of things that are arrays are still uncovered. The user settings store is working around this by checking manually, and the cluster and workspace stores work around this by using immutable versions of the data structures they were using in the first place.

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

WEB-1762

netlify · 2023-11-13T21:52:02Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`d3546ba`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/655d0f805932c20008f816d1
😎 Deploy Preview	https://deploy-preview-8405--determined-ui.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

ClaireNeveu · 2023-11-16T14:46:52Z

webui/react/src/stores/cluster.tsx

-  #resourcePools: WritableObservable<Loadable<ResourcePool[]>> = observable(NotLoaded);
-  #unboundResourcePools: WritableObservable<Loadable<ResourcePool[]>> = observable(NotLoaded);
-  #resourcePoolBindings: WritableObservable<Map<string, number[]>> = observable(Map());
+  #agents = deepObservable<Loadable<Agent[]>>(NotLoaded);


With the introduction of asValueObject and immutableObservable it does open a question of whether we should be using List from immutable.js.

the top value being observed here isn't an immutable collection, though -- it's a Loadable of an immutable collection. The issue here is that neither observable type works for that combo. deepObservable will trip over the immutable value because two collections that have the same values can have different state, and without a way of describing a immutable collection in io-ts, there isn't a way of detecting that when putting together the equality function in asValueObject.

We should add the equals and hashCode methods to Loadable. Since it's a class we can do that easily.

without a way of describing a immutable collection in io-ts

Worth noting that this would be pretty easy to add. Obviously out of scope for this PR but I think it would increase consistency.

ClaireNeveu · 2023-11-16T14:48:44Z

webui/react/src/stores/cluster.tsx


  public readonly agents = this.#agents.readOnly();
-  public readonly resourcePoolBindings = this.#resourcePoolBindings.readOnly();
+  public readonly resourcePoolBindings = this.#resourcePoolBindings.select((bindings) =>
+    bindings.map((workspaceIds) => workspaceIds.toJS()),


Any particular reason you didn't want to return an OrderedSet?

For this pr, I wanted to make sure the only changes were to the bookkeeping done inside the stores -- external types should remain the same so we know that if bugs occur it's because of the update checking, not the type change.

webui/react/src/stores/experiments.tsx

ClaireNeveu · 2023-11-16T14:54:23Z

webui/react/src/stores/permissions.tsx

@@ -1,47 +1,45 @@
 import { Loadable, Loaded, NotLoaded } from 'hew/utils/loadable';


@mapmeld Can you review this file?

OK I checked permissions were working as expected on OSS (Model Registry) and EE (adding and viewing members of workspaces as admin or general user)

ClaireNeveu · 2023-11-16T14:57:51Z

webui/react/src/utils/asValueObject.test.ts

+const otherExtra = asValueObject(codec, { ...testVal, extra: 2 } as any);
+const map = Map({ key: valueObj });
+
+describe('asValueObject', () => {


These tests are a good case for fast-check.

ClaireNeveu · 2023-11-20T14:38:45Z

webui/react/src/types.ts

 export type JsonArray = Array<Json>;
+export const JsonArrayC: t.Type<JsonArray> = t.recursion('JsonArray', () => t.array(JsonC));


The runtime types should have the same name as the compile-time types.

EmilyBonar

Rebase off main to fix the build error with Json

EmilyBonar · 2023-11-21T17:05:05Z

webui/react/src/types.ts

+  t.record(t.string, Json),
+);
+
+// export type Json = string | number | boolean | null | JsonArray | JsonObject;


remember to remove comment

this introduces two patterns that make it easier for developers to work with observable values without introducing extra renders and events: 1) `deepObservable` and `immutableObservable`: These observables do an extra check before setting the observable value. `deepObservable` does a deep equality check, and `immutableObservable` uses `Immutable.is`. If the old and new values are the same via this check, we throw out the new value. 2) `asValueObject`: the `immutableObservable` type from above by itself isn't enough to solve the problem for our usage of immutables, becuase we use plain javascript objects and arrays inside immutable objects. By default, immutable uses reference equality to determine if two immutable collections are the same, e.g. ```ts import { is, Map } from 'immutable' const map = Map({ 'hi': 'hello' }) is(map.set('hi', 'hello'), map) // true const newMap = map.set('bye', {}) is(newMap.set('bye', {}), newMap) // false ``` however, if an object implements the functions `hashCode: () => number` and `equals: (v: unknown) => boolean`, `Immutable.is` will use those functions to determine whether or not two objects are the same. To facilitate doing this, we introduce `asValueObject`. `asValueObject` takes a codec and a object implementing that codec and returns a version of the value implementing `ValueObject`. The resulting value object uses the codec to determine which values on the object to check. Checking happens as deep as the codec goes.

cla-bot bot added the cla-signed label Nov 13, 2023

ashtonG marked this pull request as ready for review November 15, 2023 17:40

ashtonG requested a review from a team as a code owner November 15, 2023 17:40

ashtonG requested review from ClaireNeveu and EmilyBonar November 15, 2023 17:40

ClaireNeveu reviewed Nov 16, 2023

View reviewed changes

ClaireNeveu suggested changes Nov 16, 2023

View reviewed changes

ClaireNeveu reviewed Nov 20, 2023

View reviewed changes

ashtonG requested a review from ClaireNeveu November 20, 2023 16:35

ClaireNeveu approved these changes Nov 21, 2023

View reviewed changes

EmilyBonar reviewed Nov 21, 2023

View reviewed changes

ashtonG added 14 commits November 21, 2023 13:19

use immutableObservable in experiments store

970f25b

fix import

4b9daf6

erase valueof from external facing observables

9c9f48d

fix build issues

159e612

slight cleanup of cluster store

52e07e0

prevent breaking valueobjects when spreading

63a63e7

add tests for extra props and any/unknown checks

07135f4

move json codecs into types.ts

2d4700d

name codecs the same as their types

7bf9b9c

use fast-check

28a8849

make sure json types are accurate

e5c77f7

fix build failure

2e662f2

fix NaN equality, test unions, remove comment

646fb94

ashtonG force-pushed the chore/WEB-1762/deep-observables branch from 5322110 to 646fb94 Compare November 21, 2023 19:20

fix ts error

d3546ba

ashtonG merged commit 16ea5a0 into main Nov 21, 2023
69 of 80 checks passed

ashtonG deleted the chore/WEB-1762/deep-observables branch November 21, 2023 21:24

ashtonG mentioned this pull request Dec 14, 2023

fix: prevent theme settings conflict in multi-client/window scenario #8596

Merged

4 tasks

dannysauer added this to the 0.26.5 milestone Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: introduce and use observables with improved update checking #8405

chore: introduce and use observables with improved update checking #8405

ashtonG commented Nov 13, 2023 •

edited

Loading

netlify bot commented Nov 13, 2023 •

edited

Loading

ClaireNeveu Nov 16, 2023

ashtonG Nov 16, 2023

ClaireNeveu Nov 16, 2023

ClaireNeveu Nov 16, 2023

ClaireNeveu Nov 16, 2023

ashtonG Nov 16, 2023

ClaireNeveu Nov 16, 2023

mapmeld Nov 16, 2023

ClaireNeveu Nov 16, 2023

ClaireNeveu Nov 20, 2023

EmilyBonar left a comment

EmilyBonar Nov 21, 2023

		@@ -1,47 +1,45 @@
		import { Loadable, Loaded, NotLoaded } from 'hew/utils/loadable';

		export type JsonArray = Array<Json>;
		export const JsonArrayC: t.Type<JsonArray> = t.recursion('JsonArray', () => t.array(JsonC));

chore: introduce and use observables with improved update checking #8405

chore: introduce and use observables with improved update checking #8405

Conversation

ashtonG commented Nov 13, 2023 • edited Loading

Description

Test Plan

Commentary (optional)

Checklist

Ticket

netlify bot commented Nov 13, 2023 • edited Loading

✅ Deploy Preview for determined-ui ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmilyBonar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashtonG commented Nov 13, 2023 •

edited

Loading

netlify bot commented Nov 13, 2023 •

edited

Loading