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

Decouple build feature code #838

Merged
merged 8 commits into from
Nov 23, 2022
Merged

Decouple build feature code #838

merged 8 commits into from
Nov 23, 2022

Conversation

xiaoyongzhu
Copy link
Member

@xiaoyongzhu xiaoyongzhu commented Nov 7, 2022 •
edited
Loading

Description

This PR decouples the save_to_feature_config_from_context method from the registry code, given:

  1. It doesn't make sense to have this method in the registry code
  2. This method introduces unnecessary dependencies for registry, which makes a few other functionalities have dependencies on feature registry, such as local spark runner/multi cloud support.

How was this PR tested?

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.

@xiaoyongzhu xiaoyongzhu added the safe to test Tag to execute build pipeline for a PR from forked repo label Nov 7, 2022
Yuqing-cat
Yuqing-cat reviewed Nov 23, 2022
View reviewed changes
Copy link
Collaborator

@Yuqing-cat Yuqing-cat left a comment

Choose a reason for hiding this comment

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

Looks good to me, add some comments.

feathr_project/feathr/definition/config_helper.py Outdated Show resolved Hide resolved
feathr_project/feathr/client.py Show resolved Hide resolved
feathr_project/feathr/definition/config_helper.py Show resolved Hide resolved
@xiaoyongzhu xiaoyongzhu merged commit 799fac0 into feathr-ai:main Nov 23, 2022
@xiaoyongzhu xiaoyongzhu deleted the decouple_build_feature branch November 23, 2022 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabbasi-hbo aabbasi-hbo aabbasi-hbo left review comments

@Yuqing-cat Yuqing-cat Yuqing-cat approved these changes

@blrchen blrchen blrchen approved these changes

@windoze windoze Awaiting requested review from windoze

@jaymo001 jaymo001 Awaiting requested review from jaymo001

@edwincheung edwincheung Awaiting requested review from edwincheung

@enya-yx enya-yx Awaiting requested review from enya-yx

@hangfei hangfei Awaiting requested review from hangfei

@jainr jainr Awaiting requested review from jainr

Assignees
No one assigned
Labels
safe to test Tag to execute build pipeline for a PR from forked repo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xiaoyongzhu @blrchen @Yuqing-cat @aabbasi-hbo