-
Notifications
You must be signed in to change notification settings - Fork 238
Added feature validation #258
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
Added feature validation #258
Conversation
hangfei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for the contribution!
|
Once the tests passed, we can merge this change. |
|
build is failing for reason unrelated to change |
|
The one you shared seems to be warnings. The root cause is this failed test: I am re-running the tests to see if it's transistent. |
|
The feature name f_snowflake_call_center_division_name and f_snowflake_call_center_zipcode seems good. It should pass your validation. Doesn't seem to be related to your change. |
|
It seems the failure exist in main branch. Let me do some research to fix it. |
Sounds good. I will merge main branch into this PR branch once the main build is green. |
|
@lhayhurst I looked thru the test failures and they are unrelated with the changes that you made. I have a small PR for those (#276) in case you are interested in. I went ahead and merged the code. Thanks for the contribution! |
* 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) ...
See this issue, "Add validation for feature name".
This PR adds a new method to
feathr_project/feathr/feature.py,FeatureBase.validate_feature_name, that validates the provided feature name against the following rule:Unit tests are provided for this new function, which is called from the
FeatureBaseconstructor before setting theself.name.I drafted a (now closed) PR against my fork, lhayhurst#1, in order to validate that this PR was headed in the right directi. @hangfei gave me feedback in that PR, which I've incorporated here.