-
Notifications
You must be signed in to change notification settings - Fork 149
Added delete_namespace and delete_project functions
#1318
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 7 commits
46ebbbf
9e4bab3
9d93604
b4e9586
9ad3e34
21e94a0
51d93d2
31fd2e9
fab307a
6b86ba2
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 |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| UniqueConstraint, | ||
| select, | ||
| ) | ||
| from sqlalchemy.sql import func as f | ||
|
|
||
| from datachain.data_storage import JobQueryType, JobStatus | ||
| from datachain.data_storage.serializer import Serializable | ||
|
|
@@ -37,7 +38,9 @@ | |
| from datachain.error import ( | ||
| DatasetNotFoundError, | ||
| DatasetVersionNotFoundError, | ||
| NamespaceDeleteNotAllowedError, | ||
| NamespaceNotFoundError, | ||
| ProjectDeleteNotAllowedError, | ||
| ProjectNotFoundError, | ||
| TableMissingError, | ||
| ) | ||
|
|
@@ -141,6 +144,10 @@ def create_namespace( | |
| def get_namespace(self, name: str, conn=None) -> Namespace: | ||
| """Gets a single namespace by name""" | ||
|
|
||
| @abstractmethod | ||
| def remove_namespace(self, namespace_id: int, conn=None) -> None: | ||
| """Removes a single namespace by id""" | ||
|
ilongin marked this conversation as resolved.
|
||
|
|
||
| @abstractmethod | ||
| def list_namespaces(self, conn=None) -> list[Namespace]: | ||
| """Gets a list of all namespaces""" | ||
|
|
@@ -190,10 +197,30 @@ def get_project( | |
| It also creates project if not found and create flag is set to True. | ||
| """ | ||
|
|
||
| def is_default_project(self, project_name: str, namespace_name: str) -> bool: | ||
| return ( | ||
| project_name == self.default_project_name | ||
| and namespace_name == self.default_namespace_name | ||
| ) | ||
|
|
||
| def is_listing_project(self, project_name: str, namespace_name: str) -> bool: | ||
|
Contributor
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. Can we prevent it in the db level as well? Have a flag for all special ovjects. To make it more reliable and not easy to hack
Contributor
Author
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. We could, yes, but I would like to do it in a followup as it requires more changes, DB migration etc. |
||
| return ( | ||
| project_name == self.listing_project_name | ||
| and namespace_name == self.system_namespace_name | ||
| ) | ||
|
|
||
| @abstractmethod | ||
| def get_project_by_id(self, project_id: int, conn=None) -> Project: | ||
| """Gets a single project by id""" | ||
|
|
||
| @abstractmethod | ||
| def count_projects(self, namespace_id: Optional[int] = None) -> int: | ||
| """Counts projects in some namespace or in general.""" | ||
|
|
||
| @abstractmethod | ||
| def remove_project(self, project_id: int, conn=None) -> None: | ||
| """Removes a single project by id""" | ||
|
|
||
| @abstractmethod | ||
| def list_projects(self, namespace_id: Optional[int], conn=None) -> list[Project]: | ||
| """Gets list of projects in some namespace or in general (in all namespaces)""" | ||
|
|
@@ -270,6 +297,10 @@ def list_datasets( | |
| ) -> Iterator[DatasetListRecord]: | ||
| """Lists all datasets in some project or in all projects.""" | ||
|
|
||
| @abstractmethod | ||
| def count_datasets(self, project_id: Optional[int] = None) -> int: | ||
| """Counts datasets in some project or in all projects.""" | ||
|
|
||
| @abstractmethod | ||
| def list_datasets_by_prefix( | ||
| self, prefix: str, project_id: Optional[int] = None | ||
|
|
@@ -735,6 +766,18 @@ def create_namespace( | |
|
|
||
| return self.get_namespace(name) | ||
|
|
||
| def remove_namespace(self, namespace_id: int, conn=None) -> None: | ||
| num_projects = self.count_projects(namespace_id) | ||
| if num_projects > 0: | ||
| raise NamespaceDeleteNotAllowedError( | ||
| f"Namespace cannot be removed. It contains {num_projects} project(s). " | ||
| "Please remove the project(s) first." | ||
| ) | ||
|
|
||
| n = self._namespaces | ||
| with self.db.transaction(): | ||
|
Contributor
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. suggestion: Consider handling the case where the namespace or project does not exist before attempting deletion. If no rows are deleted, raise a NotFoundError to inform callers that the namespace or project does not exist. Suggested implementation: def remove_namespace(self, namespace_id: int, conn=None) -> None:
n = self._namespaces
with self.db.transaction():
result = self.db.execute(self._namespaces_delete().where(n.c.id == namespace_id))
if result.rowcount == 0:
raise NotFoundError(f"Namespace or project with id {namespace_id} does not exist.")Make sure that from datachain.data_storage.exceptions import NotFoundErroror wherever |
||
| self.db.execute(self._namespaces_delete().where(n.c.id == namespace_id)) | ||
|
Contributor
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 am not sure, just wonder, should we add another check here (so no datasets exists within this namespace), or is it enough to have this check on a higher level? May be consider to move the check here and raise an exceptions (like "default namespace can not be deleted" or "namespace with datasets can not be deleted) to be on the safe side? 🤔
Contributor
Author
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 that check should be added here. It's a higher level business logic check and metastore, IMO should be de-coupled of that .. it should be just simple DB layer for other parts to use and that's it. For example, maybe we will have some management command to actually remove those default namespaces at some point and we want to be able to use metastore.
Contributor
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 believe it not a business logic, but DB level constraints. Since we don't have foreign keys at the DB level to prevent deletion of relations and if someone will use this Also if we'll want to use
Contributor
Author
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. Aha, I thought you were referring to removing default / system namespaces and checks for that .. sorry. Anyway, yes, I added additional check for empty namespace / project on removal, that makes sense to me. |
||
|
|
||
| def get_namespace(self, name: str, conn=None) -> Namespace: | ||
| """Gets a single namespace by name""" | ||
| n = self._namespaces | ||
|
|
@@ -796,18 +839,6 @@ def create_project( | |
|
|
||
| return self.get_project(name, namespace.name) | ||
|
|
||
| def _is_listing_project(self, project_name: str, namespace_name: str) -> bool: | ||
| return ( | ||
| project_name == self.listing_project_name | ||
| and namespace_name == self.system_namespace_name | ||
| ) | ||
|
|
||
| def _is_default_project(self, project_name: str, namespace_name: str) -> bool: | ||
| return ( | ||
| project_name == self.default_project_name | ||
| and namespace_name == self.default_namespace_name | ||
| ) | ||
|
|
||
| def get_project( | ||
| self, name: str, namespace_name: str, create: bool = False, conn=None | ||
| ) -> Project: | ||
|
|
@@ -816,7 +847,7 @@ def get_project( | |
| p = self._projects | ||
| validate = True | ||
|
|
||
| if self._is_listing_project(name, namespace_name) or self._is_default_project( | ||
| if self.is_listing_project(name, namespace_name) or self.is_default_project( | ||
| name, namespace_name | ||
| ): | ||
| # we are always creating default and listing projects if they don't exist | ||
|
|
@@ -858,7 +889,31 @@ def get_project_by_id(self, project_id: int, conn=None) -> Project: | |
| raise ProjectNotFoundError(f"Project with id {project_id} not found.") | ||
| return self.project_class.parse(*rows[0]) | ||
|
|
||
| def list_projects(self, namespace_id: Optional[int], conn=None) -> list[Project]: | ||
| def count_projects(self, namespace_id: Optional[int] = None) -> int: | ||
| p = self._projects | ||
| query = self._projects_select() | ||
| if namespace_id: | ||
| query = query.where(p.c.namespace_id == namespace_id) | ||
|
|
||
| query = select(f.count(1)).select_from(query.subquery()) | ||
|
|
||
| return next(self.db.execute(query))[0] | ||
|
|
||
| def remove_project(self, project_id: int, conn=None) -> None: | ||
| num_datasets = self.count_datasets(project_id) | ||
| if num_datasets > 0: | ||
| raise ProjectDeleteNotAllowedError( | ||
| f"Project cannot be removed. It contains {num_datasets} dataset(s). " | ||
| "Please remove the dataset(s) first." | ||
| ) | ||
|
|
||
| p = self._projects | ||
| with self.db.transaction(): | ||
| self.db.execute(self._projects_delete().where(p.c.id == project_id)) | ||
|
Contributor
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. Same question here with checks on this level 🤔
Contributor
Author
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. Same answer as above.... I personally wouldn't put these kind of things here. It's business logic |
||
|
|
||
| def list_projects( | ||
| self, namespace_id: Optional[int] = None, conn=None | ||
| ) -> list[Project]: | ||
| """ | ||
| Gets a list of projects inside some namespace, or in all namespaces | ||
| """ | ||
|
|
@@ -1189,7 +1244,6 @@ def _base_list_datasets_query(self) -> "Select": | |
| def list_datasets( | ||
| self, project_id: Optional[int] = None | ||
| ) -> Iterator["DatasetListRecord"]: | ||
| """Lists all datasets.""" | ||
| d = self._datasets | ||
| query = self._base_list_datasets_query().order_by( | ||
| self._datasets.c.name, self._datasets_versions.c.version | ||
|
|
@@ -1198,6 +1252,16 @@ def list_datasets( | |
| query = query.where(d.c.project_id == project_id) | ||
| yield from self._parse_dataset_list(self.db.execute(query)) | ||
|
|
||
| def count_datasets(self, project_id: Optional[int] = None) -> int: | ||
| d = self._datasets | ||
| query = self._datasets_select() | ||
| if project_id: | ||
| query = query.where(d.c.project_id == project_id) | ||
|
|
||
| query = select(f.count(1)).select_from(query.subquery()) | ||
|
|
||
| return next(self.db.execute(query))[0] | ||
|
|
||
| def list_datasets_by_prefix( | ||
| self, prefix: str, project_id: Optional[int] = None, conn=None | ||
| ) -> Iterator["DatasetListRecord"]: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| from typing import Optional | ||
|
|
||
| from datachain.error import NamespaceCreateNotAllowedError | ||
| from datachain.error import ( | ||
| NamespaceCreateNotAllowedError, | ||
| NamespaceDeleteNotAllowedError, | ||
| ) | ||
| from datachain.namespace import Namespace | ||
| from datachain.query import Session | ||
|
|
||
|
|
@@ -71,3 +74,49 @@ def ls(session: Optional[Session] = None) -> list[Namespace]: | |
| ``` | ||
| """ | ||
| return Session.get(session).catalog.metastore.list_namespaces() | ||
|
|
||
|
|
||
| def delete(name: str, session: Optional[Session]) -> None: | ||
| """ | ||
| Removes a namespace by name. | ||
|
|
||
| Raises: | ||
| NamespaceNotFoundError: If the namespace does not exist. | ||
| NamespaceDeleteNotAllowedError: If the namespace is non-empty, | ||
| is the default namespace, or is a system namespace, | ||
| as these cannot be removed. | ||
|
|
||
| Parameters: | ||
| name : The name of the namespace. | ||
| session : Session to use for getting project. | ||
|
|
||
| Example: | ||
| ```py | ||
| import datachain as dc | ||
| from datachain.lib.namespace import delete as delete_namespace | ||
|
ilongin marked this conversation as resolved.
|
||
| delete_namespace("dev") | ||
| ``` | ||
| """ | ||
| session = Session.get(session) | ||
| metastore = session.catalog.metastore | ||
|
|
||
| namespace = metastore.get_namespace(name) | ||
|
|
||
| if name == metastore.system_namespace_name: | ||
| raise NamespaceDeleteNotAllowedError( | ||
| f"Namespace {metastore.system_namespace_name} cannot be removed" | ||
| ) | ||
|
|
||
| if name == metastore.default_namespace_name: | ||
| raise NamespaceDeleteNotAllowedError( | ||
| f"Namespace {metastore.default_namespace_name} cannot be removed" | ||
| ) | ||
|
|
||
| num_projects = metastore.count_projects(namespace.id) | ||
| if num_projects > 0: | ||
| raise NamespaceDeleteNotAllowedError( | ||
| f"Namespace cannot be removed. It contains {num_projects} project(s). " | ||
| "Please remove the project(s) first." | ||
| ) | ||
|
Comment on lines
+121
to
+126
Contributor
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. We can now remove this check from here. |
||
|
|
||
| metastore.remove_namespace(namespace.id) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| from typing import Optional | ||
|
|
||
| from datachain.error import ProjectCreateNotAllowedError | ||
| from datachain.error import ProjectCreateNotAllowedError, ProjectDeleteNotAllowedError | ||
| from datachain.project import Project | ||
| from datachain.query import Session | ||
|
|
||
|
|
@@ -86,3 +86,49 @@ def ls( | |
| namespace_id = session.catalog.metastore.get_namespace(namespace).id | ||
|
|
||
| return session.catalog.metastore.list_projects(namespace_id) | ||
|
|
||
|
|
||
| def delete(name: str, namespace: str, session: Optional[Session] = None) -> None: | ||
| """ | ||
| Removes a project by name within a namespace. | ||
|
|
||
| Raises: | ||
| ProjectNotFoundError: If the project does not exist. | ||
| ProjectDeleteNotAllowedError: If the project is non-empty, | ||
| is the default project, or is a listing project, | ||
| as these cannot be removed. | ||
|
|
||
| Parameters: | ||
| name : The name of the project. | ||
| namespace : The name of the namespace. | ||
| session : Session to use for getting project. | ||
|
|
||
| Example: | ||
| ```py | ||
| import datachain as dc | ||
| dc.delete_project("my-project", "local") | ||
| ``` | ||
| """ | ||
| session = Session.get(session) | ||
| metastore = session.catalog.metastore | ||
|
|
||
| project = metastore.get_project(name, namespace) | ||
|
|
||
| if metastore.is_listing_project(name, namespace): | ||
|
Contributor
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. This checks should be also happening on the API side + on the db level. |
||
| raise ProjectDeleteNotAllowedError( | ||
| f"Project {metastore.listing_project_name} cannot be removed" | ||
| ) | ||
|
|
||
| if metastore.is_default_project(name, namespace): | ||
| raise ProjectDeleteNotAllowedError( | ||
| f"Project {metastore.default_project_name} cannot be removed" | ||
| ) | ||
|
|
||
| num_datasets = metastore.count_datasets(project.id) | ||
| if num_datasets > 0: | ||
| raise ProjectDeleteNotAllowedError( | ||
| f"Project cannot be removed. It contains {num_datasets} dataset(s). " | ||
| "Please remove the dataset(s) first." | ||
| ) | ||
|
Comment on lines
+127
to
+132
Contributor
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. We can now remove this check from here. |
||
|
|
||
| metastore.remove_project(project.id) | ||
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.
Let's don't expose project in user's api. Instead you can do something like:
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.
I didn't want to do this here as there is an open issue about this to change it everywhere (and be consistent) #1302 ..
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.
Not sure I follow 🙂
We’re exposing an API that’ll be removed soon anyway. Skipping it would just mean ~5 extra lines of code.
Or am I missing something?
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.
I wanted to be consistent in all of our APIs and change them all together, but ok, we can do this one in correct way right away. I changed it, please approve if you think this is ok now.