Skip to content

Conversation

@gtarpenning
Copy link
Member

@gtarpenning gtarpenning commented Jul 18, 2024

should fix: https://wandb.atlassian.net/browse/WB-19245
depends on: https://github.com/wandb/core/pull/22755

This pr:

  • Returns a string instead of a ref when the project ID is None

We display the string, as it becomes the op_name. We can either handle that case specially in the frontend, or choose this string discerningly.

private-ref

@gtarpenning gtarpenning marked this pull request as ready for review July 18, 2024 19:55
@gtarpenning gtarpenning requested a review from a team as a code owner July 18, 2024 19:55

WEAVE_INTERNAL_SCHEME = "weave-trace-internal"
WEAVE_SCHEME = "weave"
PRIVATE_WEAVE_STR = "<private>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use something more distinct, for example <<PRIVATE_REF_DATA>>

Copy link
Collaborator

Choose a reason for hiding this comment

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

i wonder if we should do something like:
weave-private://///{rest} (yes, 5 slashes).

Copy link
Collaborator

Choose a reason for hiding this comment

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

that would still meet our criteria of not being an official "ref", but also retain the "rest" component (name, version, etc...) if we did want to parse it in the future

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I like it

@circle-job-mirror
Copy link

circle-job-mirror bot commented Jul 18, 2024

Comment on lines 21 to 27
export const isRef = (value: any): boolean => {
return (
typeof value === 'string' &&
(value.startsWith(WANDB_ARTIFACT_REF_PREFIX) ||
value.startsWith(WEAVE_REF_PREFIX))
);
if (typeof value !== 'string') {
return false;
}
if (value.startsWith(WEAVE_REF_PREFIX_EMPTY)) {
return false;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

not technically a ref, don't try to parse it

Comment on lines 1330 to 1338
export const emptyRefToSimpleName = (ref: string) => {
const trimmed = ref.replace(WEAVE_REF_PREFIX_EMPTY, '');
try {
return trimmed.split('/')[1].split(':')[0];
} catch (e) {
return trimmed;
}
};

Copy link
Member Author

Choose a reason for hiding this comment

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

simple parse for our empty ref that goes from the fake URI to just the object_id

req.project_id = self._idc.ext_to_int_project_id(original_project_id)
res = self._ref_apply(self._internal_trace_server.ops_query, req)
for op in res.ops:
for op in res.op_objs:
Copy link
Member Author

Choose a reason for hiding this comment

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

I was getting type errors here, but i dont understand how this is working...

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

we don't implement these on server so no one every hit them

)
external_project_id = int_to_ext_project_cache[project_id]
if not external_project_id:
return f"{ri.WEAVE_PRIVATE_SCHEME}://///{tail}"
Copy link
Member Author

Choose a reason for hiding this comment

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

empty entity/project

@gtarpenning gtarpenning requested a review from tssweeney July 23, 2024 21:26
@gtarpenning gtarpenning merged commit b73c7b3 into master Jul 23, 2024
@gtarpenning gtarpenning deleted the griffin/private-ref-str branch July 23, 2024 21:59
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants