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

Yihui/moderate registration conflict #304

Merged
merged 11 commits into from
Jun 1, 2022
Merged

Yihui/moderate registration conflict #304

merged 11 commits into from
Jun 1, 2022

Conversation

YihuiGuo
Copy link
Contributor

@YihuiGuo YihuiGuo commented May 30, 2022 •
edited
Loading

This PR merges existing anchor with new anchor during registration in order to fix concurrent conflict.
Will serve as a quick fix, but will not solved it entirely.
Full fix will work with MVCC, and is in progress.

The use case is :

  1. Declare and register features, for one anchor, do not put all the features inside, only part of the features.
  2. Initialte another feathr client, declare and register the other part of the features.
  3. Call get_feature_from_registry, the second registration overwrites the first registration. i.e. only the second part is present.

@YihuiGuo
Copy link
Contributor Author

Related issue: #263

@xiaoyongzhu xiaoyongzhu added the safe to test Tag to execute build pipeline for a PR from forked repo label May 30, 2022
@xiaoyongzhu xiaoyongzhu linked an issue May 30, 2022 that may be closed by this pull request
get_features_from_registry not getting all the features #263
Closed
xiaoyongzhu
xiaoyongzhu reviewed May 31, 2022
View reviewed changes
feathr_project/feathr/_feature_registry.py Outdated 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 Outdated Show resolved Hide resolved
xiaoyongzhu
xiaoyongzhu approved these changes Jun 1, 2022
View reviewed changes
feathr_project/feathr/_feature_registry.py Outdated Show resolved Hide resolved
Yuqing-cat
Yuqing-cat approved these changes Jun 1, 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 as a short-term fix.

feathr_project/test/test_fixture.py Show resolved Hide resolved
@xiaoyongzhu xiaoyongzhu merged commit 28762c9 into feathr-ai:main Jun 1, 2022
bozhonghu pushed a commit that referenced this pull request Jun 1, 2022
@bobohu97
Merge branch 'main' into fcm_feathr_merge
2167079 
* main: (30 commits)
  Yihui/moderate registration conflict (#304)
  Update homepage (#310)
  Add extensible extractor APIs (#302)
  Remove Java and JS from Code Scanning
  Create codeql-analysis.yml
  [feathr] Add API to materialize features to offline store (#294)
  Improve error message when path is not supported (#257)
  Add tech talk slides for Feathr (#296)
  Update README.md
  Add milestone link (#286)
  Fix millisecond timestamp handling (#288)
  Consolidating CI pipelines (#280)
  Fixed dependecy problem of pretty print utils (#273)
  Fixing a broken link in README.md (#277)
  Fix test failure (#276)
  Added feature validation (#258)
  Feathr UI: Display feature key and transform expression in feature detail pages (#262)
  Feathr UI: enable multiple tenant auth (#266)
  Reduce feathr web api docker image build time (#261)
  Pretty-print the features produced by buildFeatures  (#214)
  ...
@Yuqing-cat Yuqing-cat mentioned this pull request Nov 30, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaymo001 jaymo001 jaymo001 left review comments

@xiaoyongzhu xiaoyongzhu xiaoyongzhu approved these changes

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

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.

get_features_from_registry not getting all the features
4 participants
@YihuiGuo @xiaoyongzhu @Yuqing-cat @jaymo001