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.

Sign up for GitHub

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

Jump to bottom

Add REST API support for feature registry #99

Merged
merged 20 commits into from
Apr 16, 2022
Merged

Add REST API support for feature registry #99

merged 20 commits into from
Apr 16, 2022

Conversation

jainr
Copy link
Collaborator

@jainr jainr commented Mar 29, 2022

Adding API support to support Feature Consumer use cases like getting feature, searching etc.

Yuqing-cat
Yuqing-cat reviewed Mar 31, 2022
View reviewed changes
feathr_project/feathr/_feature_registry.py Show resolved Hide resolved
feathr_project/feathr/api.py Outdated Show resolved Hide resolved
feathr_project/feathr/_feature_registry.py Outdated Show resolved Hide resolved
@xiaoyongzhu
Copy link
Member

Overall this is the right direction, and @jainr please also update docs in the dev docs folder to document how to set it up etc.

@jainr
Copy link
Collaborator Author

jainr commented Apr 7, 2022

Overall this is the right direction, and @jainr please also update docs in the dev docs folder to document how to set it up etc.

Added dev doc on how to build API container, run it locally, push the image to Azure Container

xiaoyongzhu
xiaoyongzhu previously approved these changes Apr 11, 2022
View reviewed changes
feathr_project/feathr/api.py Outdated Show resolved Hide resolved
feathr_project/feathr/api.py Outdated Show resolved Hide resolved
docs/how-to-guides/deploy-feathr-api-as-webapp.md Show resolved Hide resolved
@xiaoyongzhu
Copy link
Member

Treat this as a skeleton and overall I'm good with it. Please coordinate with Yihui on the follow up PR

@jainr
Copy link
Collaborator Author

jainr commented Apr 11, 2022

Thanks for the review @xiaoyongzhumsft , I just addressed your comments, Agree that this is not the final state of API and subsequent PR are going to address few other comments around error handling etc.

@jainr jainr requested a review from windoze April 11, 2022 22:13
xiaoyongzhu
xiaoyongzhu previously approved these changes Apr 11, 2022
View reviewed changes
hangfei
hangfei reviewed Apr 11, 2022
View reviewed changes
Copy link
Collaborator

@hangfei hangfei left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed user/dev guide. Great work!

docs/how-to-guides/deploy-feathr-api-as-webapp.md Outdated Show resolved Hide resolved
docs/how-to-guides/deploy-feathr-api-as-webapp.md Show resolved Hide resolved
docs/how-to-guides/deploy-feathr-api-as-webapp.md Show resolved Hide resolved
feathr_project/feathr/_feature_registry.py Show resolved Hide resolved
feathr_project/feathr/_feature_registry.py Outdated Show resolved Hide resolved
feathr_project/feathr/_feature_registry.py Show resolved Hide resolved
feathr_project/feathr/_feature_registry.py Show resolved Hide resolved
feathr_project/feathr/api/api.py Show resolved Hide resolved
feathr_project/feathr/api/api.py Show resolved Hide resolved
feathr_project/feathr/api/requirements.txt Show resolved Hide resolved
@xiaoyongzhu
Copy link
Member

BTW we should make the PR title something like "Add REST API support for feature registry" to make it a bit more focused

@jainr jainr changed the title Adding REST API Support Add REST API support for feature registry Apr 12, 2022
xiaoyongzhu
xiaoyongzhu previously approved these changes Apr 14, 2022
View reviewed changes
@jainr
Copy link
Collaborator Author

jainr commented Apr 14, 2022

Addressed all the comments and fixed merge conflicts.

@jainr jainr merged commit 2aaf979 into main Apr 16, 2022
@xiaoyongzhu xiaoyongzhu deleted the rijai/api branch August 22, 2022 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yuqing-cat Yuqing-cat Yuqing-cat left review comments

@hangfei hangfei hangfei approved these changes

@xiaoyongzhu xiaoyongzhu xiaoyongzhu approved these changes

@edwincheung edwincheung Awaiting requested review from edwincheung

@jaymo001 jaymo001 Awaiting requested review from jaymo001

@windoze windoze Awaiting requested review from windoze

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jainr @xiaoyongzhu @hangfei @Yuqing-cat