feat(ui): object comparison #2942

jamie-rasmussen · 2024-11-08T12:53:06Z

Description

Adds initial implementation of object/call comparison view.

Testing

How was this PR tested?

circle-job-mirror · 2024-11-08T13:05:59Z

Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=9eac986c93204d148e8c78e46290a93ffdb89f89

tssweeney · 2024-11-19T22:51:43Z

weave-js/src/components/PagePanelComponents/Home/Browse3/context.tsx

+        .join('&');
+      return `${projectRoot(entityName, projectName)}/compare?${params}`;
+    },
+    compareObjectsUri: (


Are Ops supported as objects?

I see by playing with it that they are not - seem like we could support that in the future

Yes, that would be a good future enhancement to make things more consistent. Comparing calls was already scope creep I couldn't resist. :-)

tssweeney · 2024-11-19T22:52:02Z

weave-js/src/components/PagePanelComponents/Home/Browse3/context.tsx

+      callIds: string[]
+    ) => {
+      const params = callIds
+        .map(id => 'call=' + encodeURIComponent(id))


Curious why you went with IDs over refs?

seems like you could do ref=... and the ref itself encodes that it is an object or a call. which would unify the model a bit.

It seems a little odd to me that the search params are mutually exclusive

Using refs would make the URLs substantially longer and uglier

e.g. comparing three calls here:
http://localhost:9000/wandb-designers/signal-maven/weave/compare?call=491f11f4-cccf-4f0b-b447-897254fa1534&call=d1e494c4-737f-49a8-be89-7703fec23b7b&call=2b58c817-607b-48ed-b4ce-4ec4fb816e95
v.s. with refs it would be:
http://localhost:9000/wandb-designers/signal-maven/weave/compare?ref=weave%3A%2F%2F%2Fwandb-designers%2Fsignal-maven%2Fcall%2F491f11f4-cccf-4f0b-b447-897254fa1534&ref=weave%3A%2F%2F%2Fwandb-designers%2Fsignal-maven%2Fcall%2Fd1e494c4-737f-49a8-be89-7703fec23b7b&ref=weave%3A%2F%2F%2Fwandb-designers%2Fsignal-maven%2Fcall%2F2b58c817-607b-48ed-b4ce-4ec4fb816e95

tssweeney · 2024-11-19T22:53:57Z

weave-js/src/components/PagePanelComponents/Home/Browse3/compare/refUtil.ts

+
+// When we replace a ref with its resolved data we insert this key
+// with the ref URI to keep track of the original value.
+export const RESOLVED_REF_KEY = '_ref';


I think we do something similar for call expansion - maybe an opportunity for some merging.

Yeah, I think there's a lot of opportunity for merging functionality, though with all of the problems we have with things like import cycle I'm a little wary of introducing dependencies across different parts of the code and willing to live with more duplication than I'd prefer.

tssweeney · 2024-11-19T22:54:24Z

weave-js/src/components/PagePanelComponents/Home/Browse3/compare/ComparePage.tsx

@@ -0,0 +1,70 @@
+/**


any reason you put this (i suppose the entire compare dir) outside of the pages dir?

At some point I'd like to do a more major reorganization of our UI directory structure. E.g. PagePanelComponents seems unnecessarily long when everything is already under components, it seems weird to have Home in the path to these, "Browse3" isn't a good name, etc.

"pages" seems redundant to me given all of this is under "PagePanelComponents" and I generally would rather have things organized/named by feature. But I can move it if you have a strong preference.

I agree with the need to refactor. In my mind weave-js/src/components/PagePanelComponents/Home/Browse3 is the root of our weave application and the initial refactor should be to just hoist that to a top level dir. Everything outside of that Browse3 that is not common W&B should be either removed or extracted. Given that framing, the pages dir serves as the "topmost" dir for our pages.

tssweeney · 2024-11-19T22:55:58Z

weave-js/src/components/PagePanelComponents/Home/Browse3/compare/hooks.ts

+  project: string,
+  objectVersionSpecifiers: string[]
+): ObjectVersionsResult => {
+  // TODO: Need to introduce a backend query (/objs/read?) to bulk get specific versions of objects


UGGG lets add that! You are completely right!

Filing at https://wandb.atlassian.net/browse/WB-21999 for follow up.

tssweeney · 2024-11-19T22:58:37Z

weave-js/src/components/PagePanelComponents/Home/Browse3/compare/ShoppingCart.tsx

+    if (oldIndex === newIndex) {
+      return;
+    }
+    const {search} = history.location;


I noticed this elsewhere, but figured I would add one comment: I generally stay way from embedding URL management deep in components like this. Ideally the URL management would happen in a very high level top component (Browse3) and everything below is driven by params and callbacks. The reason for this is that if we ever want to export to reports or embed in other contexts where the state is no longer URL managed, then we incur a pretty hefty refactor.

That's fair. But in the interest of getting this feature out I'm going to propose we do that pretty hefty refactor when the need arises rather than now, and I'll try to keep that in mind for the next feature.

tssweeney · 2024-11-19T23:19:02Z

weave-js/src/components/PagePanelComponents/Home/Browse3/compare/ComparePageObjectsLoaded.tsx

+                  <DropdownMenu.Trigger>
+                    <div className="cursor-pointer">
+                      <div className="flex items-center gap-10">
+                        {baselineEnabled


This toggle does not seem to do anything when there are only 2 things to compare

I will hide it when there are only two things.

tssweeney · 2024-11-19T23:21:54Z

weave-js/src/components/PagePanelComponents/Home/Browse3/compare/ComparePageObjectsLoaded.tsx

+                    disabled={first.version === 0}
+                    icon="back"
+                    variant="ghost"
+                    onClick={onPrev}


as a user, I am not sure what this does (especially when there are only 2 items?) unclear why these arrows are here, but not in the calls one (without reading the code)

When you have selected a sequential set of versions of the same object you can click these arrows to "move the sliding window" on the viewed versions. It's perhaps most intuitive and useful with only 2 items, e.g. you go from diffing v0 and v1, to v1 and v2, to v2 and v3, or vice-versa.

You could possibly come up with a similar concept for calls of the same op and previous/next execution by time, but I don't think there's a lot of value for the amount of effort that would take.

I will try updating the tooltip text to explain the buttons better.

tssweeney · 2024-11-19T23:24:02Z

weave-js/src/components/PagePanelComponents/Home/Browse3/compare/ComparePage.tsx

+  const onlyChanged = queryGetBoolean(history, 'changed', false);
+  const callIds = getParamArray(d, 'call');
+
+  if (callIds.length) {


I feel like we might want to limit the number to a reasonable set:

this pretty much halts to page

I'm going to limit it to 6 object columns in the grid and show a message. I think that will work for most cases while keeping the entire set in the shopping cart so you can select the items for a pairwise comparison.

jamie-rasmussen force-pushed the jamie/compare-everything branch 2 times, most recently from 156634a to 108cbba Compare November 15, 2024 16:12

jamie-rasmussen marked this pull request as ready for review November 15, 2024 16:38

jamie-rasmussen requested review from a team as code owners November 15, 2024 16:38

jamie-rasmussen requested a review from tssweeney November 15, 2024 16:38

jamie-rasmussen force-pushed the jamie/compare-everything branch from 108cbba to e83b577 Compare November 18, 2024 18:46

tssweeney reviewed Nov 19, 2024

View reviewed changes

jamie-rasmussen force-pushed the jamie/compare-everything branch from e83b577 to 7a249f3 Compare November 20, 2024 22:02

jamie-rasmussen requested a review from tssweeney November 20, 2024 22:03

tssweeney approved these changes Nov 21, 2024

View reviewed changes

jamie-rasmussen added 2 commits November 21, 2024 10:08

feat(ui): object comparison

b4772a8

CR feedback - Tim

33c27a2

jamie-rasmussen force-pushed the jamie/compare-everything branch from 7a249f3 to 33c27a2 Compare November 21, 2024 16:08

jamie-rasmussen merged commit 370c6ec into master Nov 21, 2024
115 checks passed

jamie-rasmussen deleted the jamie/compare-everything branch November 21, 2024 16:33

github-actions bot locked and limited conversation to collaborators Nov 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): object comparison #2942

feat(ui): object comparison #2942

jamie-rasmussen commented Nov 8, 2024

circle-job-mirror bot commented Nov 8, 2024 •

edited

Loading

tssweeney Nov 19, 2024

tssweeney Nov 19, 2024

jamie-rasmussen Nov 20, 2024

tssweeney Nov 19, 2024

tssweeney Nov 19, 2024

jamie-rasmussen Nov 20, 2024

tssweeney Nov 19, 2024

jamie-rasmussen Nov 20, 2024

tssweeney Nov 19, 2024

jamie-rasmussen Nov 20, 2024

tssweeney Nov 20, 2024

tssweeney Nov 19, 2024

jamie-rasmussen Nov 20, 2024

tssweeney Nov 19, 2024

jamie-rasmussen Nov 20, 2024

tssweeney Nov 19, 2024

jamie-rasmussen Nov 20, 2024

tssweeney Nov 19, 2024

jamie-rasmussen Nov 20, 2024

tssweeney Nov 19, 2024

tssweeney Nov 19, 2024

jamie-rasmussen Nov 20, 2024

feat(ui): object comparison #2942

feat(ui): object comparison #2942

Conversation

jamie-rasmussen commented Nov 8, 2024

Description

Testing

circle-job-mirror bot commented Nov 8, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

circle-job-mirror bot commented Nov 8, 2024 •

edited

Loading