-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(api): (1/n) datasets api clean up #1573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 43 commits
bc551e6
8592c2b
0e8a53a
02aa9a1
0e47c65
817331e
0abedd0
1d80ec7
31e3409
f840018
8942071
18de4cd
a3173e8
790b2d5
09039ec
4cc1958
4f6f0f6
772339b
b4d118f
0df3304
8a6fa41
8b80a77
78ec3d9
89885fd
a609582
7606e49
0e2a13d
cba4842
c7d741d
39f4dfb
5cb0ad7
72ccdc1
2c9d624
a568bf3
6f5df08
28b8c1c
f2d9332
a6fa3aa
5cf7779
63f1525
d9264a0
6a8bd19
cc48d9e
54a4f41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,19 +13,16 @@ | |
|
||
|
||
@json_schema_type | ||
class PaginatedRowsResult(BaseModel): | ||
class IterrowsResponse(BaseModel): | ||
""" | ||
A paginated list of rows from a dataset. | ||
|
||
:param rows: The rows in the current page. | ||
:param total_count: The total number of rows in the dataset. | ||
:param next_page_token: The token to get the next page of rows. | ||
:param data: The rows in the current page. | ||
:param next_start_index: Index into dataset for the first row in the next page. None if there are no more rows. | ||
""" | ||
|
||
# the rows obey the DatasetSchema for the given dataset | ||
rows: List[Dict[str, Any]] | ||
total_count: int | ||
next_page_token: Optional[str] = None | ||
data: List[Dict[str, Any]] | ||
next_start_index: Optional[int] = None | ||
|
||
|
||
class DatasetStore(Protocol): | ||
|
@@ -37,22 +34,21 @@ class DatasetIO(Protocol): | |
# keeping for aligning with inference/safety, but this is not used | ||
dataset_store: DatasetStore | ||
|
||
@webmethod(route="/datasetio/rows", method="GET") | ||
async def get_rows_paginated( | ||
# TODO(xiyan): there's a flakiness here where setting route to "/datasets/" here will not result in proper routing | ||
@webmethod(route="/datasetio/iterrows/{dataset_id:path}", method="GET") | ||
async def iterrows( | ||
self, | ||
dataset_id: str, | ||
rows_in_page: int, | ||
page_token: Optional[str] = None, | ||
filter_condition: Optional[str] = None, | ||
) -> PaginatedRowsResult: | ||
"""Get a paginated list of rows from a dataset. | ||
start_index: Optional[int] = None, | ||
limit: Optional[int] = None, | ||
) -> IterrowsResponse: | ||
"""Get a paginated list of rows from a dataset. Uses cursor-based pagination. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is using cursor-based pagination because this is just an index we are returning? Maybe just avoid saying this is cursor-based? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
:param dataset_id: The ID of the dataset to get the rows from. | ||
:param rows_in_page: The number of rows to get per page. | ||
:param page_token: The token to get the next page of rows. | ||
:param filter_condition: (Optional) A condition to filter the rows by. | ||
:param start_index: Index into dataset for the first row to get. Get all rows if None. | ||
:param limit: The number of rows to get. | ||
""" | ||
... | ||
|
||
@webmethod(route="/datasetio/rows", method="POST") | ||
@webmethod(route="/datasetio/append-rows/{dataset_id:path}", method="POST") | ||
async def append_rows(self, dataset_id: str, rows: List[Dict[str, Any]]) -> None: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is not flaky (which means it sometimes works vs. sometimes not) -- I think maybe you just mean "wonkiness" or "sadness"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it actually is the fact that this sometimes work and sometimes do not work if I set the route to
/datasets
. I'm suspecting it may be due to the way we do topological sort in our resolver