DNM: fix: Clients now correctly quote/unquote ref parts #2145

tssweeney · 2024-08-15T22:36:36Z

Summary: This PR hopefully 🤞 solves >90% of our remaining Ref/Naming issues - meaning users should be able to freely create entities/projects (as long as Gorilla accepts them) and name their objects/ops anything they want (baring a % symbol). Under the hood, I close a few remaining constraints / parsing issues on the server side and mostly modify the UI and Python clients to correctly read/write quoted refs.

High Level Improvements:

Python Client will always produce valid refs (ie. it correctly quotes when needed)
- Benefit to user: no more crashed python scripts when bad chars are in the name
- Note: the one case of % in the name is still illegal. We sanitize on the python client for this specific case.
Python Client is able to consume quoted refs (ie. it correctly unquotes when needed)
- Benefit to user: able to correctly walk ref data edges with bad chars in the name
UI Client will always produce valid refs (ie. it correctly encodes when needed)
- Benefit to user: able to link safely when dealing with bad characters
UI Client is able to consume encoded refs (ie. it correctly decodes when needed)
- Benefit to user: able to correctly display links with bad characters

Smaller Details:

Adds correct quoting of URLs to printing URLs
Adds server-side entity/project de-quoting

Companion PR: https://github.com/wandb/core/pull/23333

Correctly routes when parts are encoded
Correctly de-quoting encoded refs on project lookup

This PR finishes up the Ref work to ensure we support any ref the user throws at us.

Context:
In a recent PR: #2077, I implemented a more robust parser and checker to ensure Refs and Names conform to expectations. This stopped the "bleeding" where we were storing invalid Refs.

However, our two clients Python and UI had not been updated to reflect the new rules. In fact, the Python client was still creating bad refs, and the UI was overly restrictive with what it thought a ref could be. So double edge sword. As a result, users still hit crashes (either python write time or ui read time).

Moreover, our Ref parsing logic is a mess (especially in the UI) as it is spread across many locations & riddled with legacy junk.

A little terminology note: in PY, URL escaping is called quoting and in TS it is encoding. When used in this PR, they follow the language-specific convention, but logically mean the same concept.

Understanding Refs:
See https://www.notion.so/wandbai/WIP-Robust-Refs-550bf06f4a244a1ab4fdd26133952e2c for more details.

The important part to understand is the ref format:

Calls:
    weave:///<entity>/<project>/call/<callid>[extra]
Ops:
    weave:///<entity>/<project>/op/<name>:<digest>[extra]
Objects:
    weave:///<entity>/<project>/object/<name>:<digest>[extra]
Tables:
    weave:///<entity>/<project>/table/<digest>[extra]
Where:
[extra] is an optional set of slash-seperated tuples of edges. for example:
    /key/key_name_1/index/0
They always follow the pattern of [edgeName/edgeValue], where 
`edgeName` is a shortlist of 4 specific edges names.

Furthermore, entity, project, name and the edgeValues are user-controlled.

The main constraints we need to parse these refs are:

entity cannot contain slashes /
project cannot contain slashes /
name cannot contain slashes / or colons :
edgeValue cannot contain slashes /

Therefore, we need to basically URL encode/decode each of these parts in all the places that they are used

This PR basically updates the parsing and construction logic to properly encode/decode entity, project, nameandedgeValue`.
This adds server-side decoding of entity/project per this more rigorous spec

Deployment
The ideal order would be:

The companion PR (land and deploy): https://github.com/wandb/core/pull/23333
Secondly, split out the trace server changes from this PR (land and deploy)
Then we can land & deploy/release UI and python changes in any order
Make sure prod is good for few days
Rollout to dedicated

circle-job-mirror · 2024-08-15T22:37:45Z

Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=672d9cc574b1ef2a7f0e08b74e1374d110180feb

tssweeney · 2024-08-15T23:47:12Z

weave/trace_server/refs_internal.py

@@ -29,7 +29,7 @@ class InvalidInternalRef(ValueError):
    pass


-def quote_select(s: str, quote_chars: tuple[str, ...] = ("/", ":", "%")) -> str:
+def quote_select(s: str, quote_chars: tuple[str, ...] = ("%", "/", ":")) -> str:


This is a legit bug fix!

tssweeney · 2024-08-16T01:26:26Z

weave-js/src/components/PagePanelComponents/Home/Browse2/browse2Util.ts

@@ -7,7 +7,7 @@ export const flattenObjectPreservingWeaveTypes = (obj: {
  [key: string]: any;
 }) => {
  return flattenObject(obj, '', {}, (key, value) => {
-    return typeof value !== 'object' || value == null || value._type == null;
+    return typeof value !== 'object' || value == null || value._type !== 'CustomWeaveType';


This is an unrelated fix I stumbled upon while testing

tssweeney · 2024-08-16T01:31:02Z

weave-js/src/components/PagePanelComponents/Home/Browse2/browse2Util.ts

@@ -7,7 +7,11 @@ export const flattenObjectPreservingWeaveTypes = (obj: {
  [key: string]: any;
 }) => {
  return flattenObject(obj, '', {}, (key, value) => {
-    return typeof value !== 'object' || value == null || value._type == null;
+    return (


This is a little followup fix from the Image PR i just put in - it is unrelated to refs

tssweeney · 2024-08-16T01:32:04Z

weave-js/src/components/PagePanelComponents/Home/Browse3/pages/wfReactInterface/utilities.ts

@@ -48,8 +49,13 @@ export const refUriToOpVersionKey = (refUri: RefUri): OpVersionKey => {
 };

 export const opVersionKeyToRefUri = (key: OpVersionKey): RefUri => {
-  return `${WEAVE_REF_PREFIX}${key.entity}/${key.project}/op/${key.opId}:${key.versionHash}`;
-  // return `${WANDB_ARTIFACT_REF_PREFIX}${key.entity}/${key.project}/${key.opId}:${key.versionHash}/obj`;
+  return makeRefObject(


We have too much ref construction / parsing logic floating around, centralizing to one location.

tssweeney · 2024-08-16T01:33:10Z

weave-js/src/components/PagePanelComponents/Home/Browse3/refs.ts

@@ -0,0 +1,56 @@
+import {


much of this file already existed - moved from weave-js/util/refs.ts to here as this is scoped to "New Weave" (aka Browse3)

tssweeney · 2024-08-16T01:35:04Z

weave-js/src/util/refs.ts

Moved to weave-js/src/components/PagePanelComponents/Home/Browse3/refs.ts

tssweeney · 2024-08-16T01:35:31Z

weave/init_message.py

-    wandb._wandb_module = "weave"
-    weave_messages = wandb.sdk.internal.update.check_available(weave.__version__)
-    wandb._wandb_module = orig_module
+    weave_messages = None


This is unrelated to Refs... for some reason mypy was complaining about this.

tssweeney · 2024-08-16T01:36:21Z

weave/trace/refs.py

@@ -1,4 +1,5 @@
 import dataclasses


Major changes in this file are to ensure that we quote the user-controlled fields, similar to refs_internal

tssweeney · 2024-08-16T07:32:15Z

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

@@ -0,0 +1,19 @@
+import {ErrorPanel} from '@wandb/weave/components/ErrorPanel';


I noticed while testing that it is pretty lame when a not found error crashes the whole page. This is a common component that can be used to render not found data

tssweeney · 2024-08-16T07:32:34Z

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

@@ -92,8 +92,13 @@ export const browse2Context = {
  entityUrl: (entityName: string) => {


Pretty much this whole file is all about encoding the url parts

tssweeney · 2024-08-16T07:33:22Z

weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallPage/CallPage.tsx

-        </div>
-      </div>
-    );
+    return <NotFoundPanel title="Call not found" />;


Moved to a common component

tssweeney · 2024-08-16T07:34:31Z

...s/src/components/PagePanelComponents/Home/Browse3/pages/wfReactInterface/tsDataModelHooks.ts

@@ -669,6 +670,13 @@ const useOpVersion = (
      };
    }

+    if (opVersionRes.obj == null) {


This block (and the one below that is similar basically handles the case when the target data is not found. before, this would create a JS error due to incorrect nulls

moved to new PR

tssweeney · 2024-08-16T08:11:50Z

weave-js/src/react.tsx

+ * artifacts or local artifacts. It is not exported currently, but is used for
+ * the new Weave branch in parseRef
+ */
+const parseWeaveTraceRef = (ref: string): WeaveObjectRef => {


This was extracted from parseRef and is easier to review with whitespace hidden

vanpelt · 2024-08-16T19:04:40Z

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

+    entityName: string,
+    projectName: string,
+    noEncode?: boolean
+  ) => string;
  typeUIUrl: (
    entityName: string,
    projectName: string,


Can't we assume entityName and projectName are always safe?

tssweeney · 2024-08-16T20:26:22Z

weave-js/src/components/PagePanelComponents/Home/Browse2/browse2Util.ts

@@ -7,7 +7,11 @@ export const flattenObjectPreservingWeaveTypes = (obj: {
  [key: string]: any;


Moved to New PR

tssweeney · 2024-08-16T20:34:58Z

weave-js/src/components/PagePanelComponents/Home/Browse3/pages/OpVersionPage.tsx

@@ -35,7 +36,7 @@ export const OpVersionPage: React.FC<{
  if (opVersion.loading) {
    return <CenteredAnimatedLoader />;
  } else if (opVersion.result == null) {
-    return <div>Op version not found</div>;
+    return <NotFoundPanel title="Op not found" />;


moved to new PR

tssweeney · 2024-08-16T20:35:02Z

weave-js/src/components/PagePanelComponents/Home/Browse3/pages/ObjectVersionPage.tsx

@@ -59,7 +60,7 @@ export const ObjectVersionPage: React.FC<{
  if (objectVersion.loading) {
    return <CenteredAnimatedLoader />;
  } else if (objectVersion.result == null) {
-    return <div>Object not found</div>;
+    return <NotFoundPanel title="Object not found" />;


moved to new PR

tssweeney · 2024-08-16T20:35:09Z

weave-js/src/components/PagePanelComponents/Home/Browse3/pages/CallPage/CallPage.tsx

-        </div>
-      </div>
-    );
+    return <NotFoundPanel title="Call not found" />;


moved to new PR

tssweeney added 3 commits August 15, 2024 15:09

Initial tests

52814ba

Fixed

73aa2d6

added more tests

c00e667

bringing over more logic

696e65d

tssweeney mentioned this pull request Aug 15, 2024

fix: relax object and project name ref restrictions #2142

Closed

tssweeney added 8 commits August 15, 2024 15:48

put back

83f7743

small change

ce7feeb

added refs

7a2e0bc

cleaning things up

3299ed0

cleaning things up

0b8f11a

cleaning things up

d13c9ba

lint

bf5c8f0

lint

1f26afb

tssweeney commented Aug 15, 2024

View reviewed changes

tssweeney added 6 commits August 15, 2024 16:55

big changes

7a427f0

more fixes

b79e8bc

merged in master

ee229fe

hopefully fix lint

4e595d7

hopefully fix lint

6e43eeb

little fix

950f3e1

tssweeney commented Aug 16, 2024

View reviewed changes

lint

8e09a88

tssweeney commented Aug 16, 2024

View reviewed changes

weave-js/src/util/refs.ts Outdated

Copy link

Collaborator Author

tssweeney Aug 16, 2024

Choose a reason for hiding this comment

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

Moved to weave-js/src/components/PagePanelComponents/Home/Browse3/refs.ts

tssweeney commented Aug 16, 2024

View reviewed changes

RC 1

54059a7

tssweeney marked this pull request as draft August 16, 2024 03:35

tssweeney added 5 commits August 15, 2024 20:37

Lint

e6d40f8

more python junk

74a38cf

added protection

5e3403a

am i actually done with implementation now?

f1f615d

lint

ec7d42b

tssweeney commented Aug 16, 2024

View reviewed changes

linting

37a599a

tssweeney commented Aug 16, 2024

View reviewed changes

tssweeney added 3 commits August 16, 2024 01:13

linting

e5bc0c5

done

7f3fa74

small test fix

0dcef94

tssweeney marked this pull request as ready for review August 16, 2024 08:58

small test fix

b974c50

This was referenced Aug 16, 2024

chore(weave): Add client-side quoting for object names #2140

Closed

chore(weave): Fix mypy #2151

Closed

tssweeney marked this pull request as draft August 16, 2024 17:40

small test fix

bf1d2ac

vanpelt reviewed Aug 16, 2024

View reviewed changes

tssweeney commented Aug 16, 2024

View reviewed changes

tssweeney changed the title ~~fix: Clients now correctly quote/unquote ref parts~~ DNM: fix: Clients now correctly quote/unquote ref parts Aug 16, 2024

tssweeney mentioned this pull request Aug 16, 2024

fix: Fixes issues with special characters in object name #2156

Merged

tssweeney closed this Aug 29, 2024

github-actions bot locked and limited conversation to collaborators Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DNM: fix: Clients now correctly quote/unquote ref parts #2145

DNM: fix: Clients now correctly quote/unquote ref parts #2145

tssweeney commented Aug 15, 2024 •

edited

Loading

circle-job-mirror bot commented Aug 15, 2024 •

edited

Loading

tssweeney Aug 15, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024 •

edited

Loading

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

vanpelt Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

tssweeney Aug 16, 2024

		@@ -0,0 +1,19 @@
		import {ErrorPanel} from '@wandb/weave/components/ErrorPanel';

		@@ -92,8 +92,13 @@ export const browse2Context = {
		entityUrl: (entityName: string) => {

		@@ -7,7 +7,11 @@ export const flattenObjectPreservingWeaveTypes = (obj: {
		[key: string]: any;

DNM: fix: Clients now correctly quote/unquote ref parts #2145

DNM: fix: Clients now correctly quote/unquote ref parts #2145

Conversation

tssweeney commented Aug 15, 2024 • edited Loading

circle-job-mirror bot commented Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tssweeney Aug 16, 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

tssweeney commented Aug 15, 2024 •

edited

Loading

circle-job-mirror bot commented Aug 15, 2024 •

edited

Loading

tssweeney Aug 16, 2024 •

edited

Loading