feat(weave): Support properly retrieving a dataset back in TS SDK#5630
feat(weave): Support properly retrieving a dataset back in TS SDK#5630chance-wnb merged 1 commit intomasterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=254b1e2e08a9b9478aa7885dfb984d3e7c1e1299 |
chance-wnb
changed the title
| }; | ||
| }, | ||
|
|
||
| objReadObjReadPost: async (req: { |
There was a problem hiding this comment.
The name of these functions is strange. It duplicated objRead, can we do better?
There was a problem hiding this comment.
While you are saying it, I am shocked too.
But all these come from the auto-gen`ed http client stubs:
weave/sdks/node/src/generated/traceServerApi.ts
Line 1541 in 66b158c
And those have existed at least for a year.
I will ask Andrew tomorrow about why things are this way. but most likely it might be the way it is.
There was a problem hiding this comment.
I investigated a bit. it turned out that we can fix that by putting some annotations on the server side FastAPI level.
Details are explained in https://weightsandbiases.slack.com/archives/C03BSTEBD7F/p1761668495870889
I'd suggest we tackle this as a tech debt item separately.
| }; | ||
|
|
||
| table = { | ||
| tableCreateTableCreatePost: async (req: { |
| // Create row entries with individual digests | ||
| const rows = req.table.rows.map(rowVal => ({ | ||
| digest: this.generateDigest(rowVal), | ||
| val: rowVal, | ||
| })); |
There was a problem hiding this comment.
Any performance concerns here?
There was a problem hiding this comment.
This is a test harness file. it will only be used in test cases. So I guess it should be fine.
| }; | ||
| }, | ||
|
|
||
| tableQueryTableQueryPost: async (req: { |
There was a problem hiding this comment.
strangeFunctionStrangeFunctionName
There was a problem hiding this comment.
lol. I will investigate, but likely we might have to leave it as is.
| * Parse a Weave URI into its components. | ||
| * | ||
| * @param uri - The weave:/// URI string to parse | ||
| * @returns Parsed URI components or null if invalid | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const parsed = parseWeaveUri('weave:///wandb/project/table/abc123...'); | ||
| * if (parsed && parsed.type === 'table') { | ||
| * console.log(parsed.digest); // 'abc123...' | ||
| * } | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
The definitive mark of vide-coded functions 😆
There was a problem hiding this comment.
Haha, yes. want me to change anything?
chance-wnb
left a comment
chance-wnb
left a comment
There was a problem hiding this comment.
@neutralino1 Thanks so much for the code review! Many things weren't known to me. I investigated a bit and replied. Please kindly have another look!
| }; | ||
| }, | ||
|
|
||
| objReadObjReadPost: async (req: { |
There was a problem hiding this comment.
While you are saying it, I am shocked too.
But all these come from the auto-gen`ed http client stubs:
weave/sdks/node/src/generated/traceServerApi.ts
Line 1541 in 66b158c
And those have existed at least for a year.
I will ask Andrew tomorrow about why things are this way. but most likely it might be the way it is.
| }; | ||
| }, | ||
|
|
||
| tableQueryTableQueryPost: async (req: { |
There was a problem hiding this comment.
lol. I will investigate, but likely we might have to leave it as is.
| * Parse a Weave URI into its components. | ||
| * | ||
| * @param uri - The weave:/// URI string to parse | ||
| * @returns Parsed URI components or null if invalid | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const parsed = parseWeaveUri('weave:///wandb/project/table/abc123...'); | ||
| * if (parsed && parsed.type === 'table') { | ||
| * console.log(parsed.digest); // 'abc123...' | ||
| * } | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
Haha, yes. want me to change anything?
chance-wnb
force-pushed
the
chance/ts_dataset
branch
from
81d6902 to
817f6ab
Compare
chance-wnb
force-pushed
the
chance/ts_dataset
branch
from
817f6ab to
8138b2f
Compare
|
The failed test case is known and irrelevant. merging. |
3 participants
Description
This PR adds support for loading of Dataset and Table objects, allowing them to be retrieved from the server on demand. It includes a new URI parser for handling different types of Weave references
The implementation ensures that dataset rows can be accessed synchronously after loading.
The PR also extends the InMemoryTraceServer to support table operations for testing scenarios.
Testing
Docs:
wandb/docs#1809