-
Notifications
You must be signed in to change notification settings - Fork 88
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
Add studio flag for rm-dataset and edit-dataset #572
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,17 +178,9 @@ def _send_request(self, route: str, data: dict[str, Any]) -> Response[Any]: | |
data = {} | ||
|
||
if not ok: | ||
logger.error( | ||
"Got bad response from Studio, content is %s", | ||
response.content.decode("utf-8"), | ||
) | ||
if response.status_code == 403: | ||
message = f"Not authorized for the team {self.team}" | ||
else: | ||
logger.error( | ||
"Got bad response from Studio, content is %s", | ||
response.content.decode("utf-8"), | ||
) | ||
message = data.get("message", "") | ||
else: | ||
message = "" | ||
|
@@ -230,6 +222,46 @@ def ls(self, paths: Iterable[str]) -> Iterator[tuple[str, Response[LsData]]]: | |
def ls_datasets(self) -> Response[LsData]: | ||
return self._send_request("datachain/ls-datasets", {}) | ||
|
||
def edit_dataset( | ||
self, | ||
name: str, | ||
new_name: Optional[str] = None, | ||
description: Optional[str] = None, | ||
labels: Optional[list[str]] = None, | ||
) -> Response[DatasetInfoData]: | ||
body = { | ||
"dataset_name": name, | ||
} | ||
|
||
if new_name is not None: | ||
body["new_name"] = new_name | ||
|
||
if description is not None: | ||
body["description"] = description | ||
|
||
if labels is not None: | ||
body["labels"] = labels # type: ignore[assignment] | ||
|
||
return self._send_request( | ||
"datachain/edit-dataset", | ||
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. related to the groups of commands, etc - let's please make API also clean
-etc
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. It could even be a single endpoint that lists on Since dataset is part of a team, ideally the URI should be 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. Added this comment to the #576 as well. We can take it as a part of refactor. |
||
body, | ||
) | ||
|
||
def rm_dataset( | ||
self, | ||
name: str, | ||
version: Optional[int] = None, | ||
force: Optional[bool] = False, | ||
) -> Response[DatasetInfoData]: | ||
return self._send_request( | ||
"datachain/rm-dataset", | ||
{ | ||
"dataset_name": name, | ||
"version": version, | ||
"force": force, | ||
}, | ||
) | ||
|
||
def dataset_info(self, name: str) -> Response[DatasetInfoData]: | ||
def _parse_dataset_info(dataset_info): | ||
_parse_dates(dataset_info, ["created_at", "finished_at"]) | ||
|
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.
[Q] Is there a reason for deviating from the behaviour implemented in #561? Why can't a user edit/remove a dataset from both local and remote in the same call?
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.
For list operation, having the option to list the datasets from both local and remote seems like a frequent operation.
But I am not sure how helpful it will edit or remove dataset from both local and remote in the same call.
I was also looking at the implementation of
pull
whereds://
prefix is used to identify if it is remote or if it is local. I don't know if it will be confusing to user though to implement it in edit and remove as well. cc. @shchekleinThere 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.
If you think about this in terms of #578 users would definitely want to be able to rename both at the same time. We want to avoid users ending up with datasets with the same identifier being named differently between local and remote as much as possible.