Skip to content
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

Expose deletion API for projects/features #852

Merged
merged 13 commits into from
Dec 1, 2022

Conversation

aabbasi-hbo
Copy link
Collaborator

@aabbasi-hbo aabbasi-hbo commented Nov 12, 2022

Description

Resolves #842

Exposes the deletion API for features/projects

How was this PR tested?

Updated unit tests

Does this PR introduce any user-facing changes?

  • No. You can skip the rest of this section.
  • Yes. Make sure to clarify your proposed changes.

Copy link
Collaborator Author

@aabbasi-hbo aabbasi-hbo left a comment

Choose a reason for hiding this comment

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

@xiaoyongzhu is there a reason why the registry purview client (https://github.com/feathr-ai/feathr/blob/main/feathr_project/feathr/registry/_feature_registry_purview.py) does not call the API specified in (https://github.com/feathr-ai/feathr/blob/main/registry/purview-registry/main.py)? It seems that API functionality has been directly implemented in the client

@aabbasi-hbo
Copy link
Collaborator Author

@xiaoyongzhu is there a reason why the registry purview client (https://github.com/feathr-ai/feathr/blob/main/feathr_project/feathr/registry/_feature_registry_purview.py) does not call the API specified in (https://github.com/feathr-ai/feathr/blob/main/registry/purview-registry/main.py)? It seems that API functionality has been directly implemented in the client

Nevermind I see that it is deprecated

@aabbasi-hbo aabbasi-hbo marked this pull request as ready for review November 14, 2022 20:20
@xiaoyongzhu xiaoyongzhu added the safe to test Tag to execute build pipeline for a PR from forked repo label Nov 16, 2022
@xiaoyongzhu
Copy link
Member

Thanks @aabbasi-hbo for the PR! This is very helpful.

@windoze @YihuiGuo can you take a look as well?

@windoze
Copy link
Member

windoze commented Nov 16, 2022

Cascading deletion seems to be a too-dangerous operation, could we have some safer alternatives?
My idea is to add another method to retrieve all downstream, so we can know the correct order to delete entity one by one

@aabbasi-hbo
Copy link
Collaborator Author

Cascading deletion seems to be a too-dangerous operation, could we have some safer alternatives? My idea is to add another method to retrieve all downstream, so we can know the correct order to delete entity one by one

@windoze which cascading operation are you referring to? The deletion uses the BFS function to get all downstream entities and deletes them through another BFS

@windoze
Copy link
Member

windoze commented Nov 18, 2022

'Cascading" means one delete request could actually delete a lot of entities without any prior notice, which could be too dangerous.
I prefer to add 2 APIs, one to get a list of entities that depend on the specific one in the correct order, and another one to delete one entity at a time, refusing to delete an entity that has any downstream.

@aabbasi-hbo
Copy link
Collaborator Author

'Cascading" means one delete request could actually delete a lot of entities without any prior notice, which could be too dangerous. I prefer to add 2 APIs, one to get a list of entities that depend on the specific one in the correct order, and another one to delete one entity at a time, refusing to delete an entity that has any downstream.

got it. yeah that makes sense - will update. Thanks for the feedback!

Copy link
Collaborator Author

@aabbasi-hbo aabbasi-hbo left a comment

Choose a reason for hiding this comment

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

@windoze mind reviewing again when you get a chance? thanks!

Copy link
Member

@windoze windoze left a comment

Choose a reason for hiding this comment

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

It looks good to me, thanks for a great job.

registry/purview-registry/main.py Outdated Show resolved Hide resolved
registry/sql-registry/main.py Outdated Show resolved Hide resolved
@windoze
Copy link
Member

windoze commented Nov 23, 2022

@windoze mind reviewing again when you get a chance? thanks!

Sorry for the delay, I was working on something else.
I've put my comments, please take a look.

Thanks for the contribution!

Copy link
Member

@xiaoyongzhu xiaoyongzhu left a comment

Choose a reason for hiding this comment

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

This is a really nice PR, thanks for the contribution!

@xiaoyongzhu xiaoyongzhu merged commit 858f88f into feathr-ai:main Dec 1, 2022
jaymo001 pushed a commit that referenced this pull request Dec 7, 2022
* registry-changes

* update purview

* remove delete functionality for now

* update tests

* remove unused import

* update endpoints

* fix locking issue

* Update _feature_registry_purview.py

* remove cascading delete

* Update feature_registry.py

* update access control

* update status code to 412
Yuqing-cat added a commit to Yuqing-cat/feathr that referenced this pull request Jan 10, 2023
Yuqing-cat added a commit that referenced this pull request Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Tag to execute build pipeline for a PR from forked repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Expose the deletion API for features/projects
3 participants