FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Expose deletion API for projects/features by aabbasi-hbo · Pull Request #852 · feathr-ai/feathr · GitHub

Expose deletion API for projects/features - #852

Merged
xiaoyongzhu merged 13 commits into
feathr-ai:mainfrom
aabbasi-hbo:delete-feature-project-api
Dec 1, 2022
Merged

Expose deletion API for projects/features#852
xiaoyongzhu merged 13 commits into
feathr-ai:mainfrom
aabbasi-hbo:delete-feature-project-api

Conversation

aabbasi-hbo commented Nov 12, 2022
edited
Loading

Copy link
Copy Markdown
Collaborator

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.

aabbasi-hbo left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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

Copy link
Copy Markdown
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 marked this pull request as ready for review November 14, 2022 20:20
xiaoyongzhu added the safe to test Tag to execute build pipeline for a PR from forked repo label Nov 16, 2022

Copy link
Copy Markdown
Member

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

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

windoze commented Nov 16, 2022

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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 commented Nov 18, 2022

Copy link
Copy Markdown
Member

'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.

Copy link
Copy Markdown
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!

aabbasi-hbo left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

windoze left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread registry/purview-registry/main.py Outdated
Comment thread registry/sql-registry/main.py Outdated

windoze commented Nov 23, 2022

Copy link
Copy Markdown
Member

@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!

xiaoyongzhu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL