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.

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

Remove support for non-entity_id grouping for features #887

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

shaycrk
Copy link
Contributor

@shaycrk shaycrk commented Mar 16, 2022

Specifying groups other than entity_id has been error-prone because it requires (but doesn't enforce/check for) a one-to-one relationship between the entities and other grouping levels (discussed in #874). As such, this PR removes support for specifying the groups key in the feature configuration (as well as removing instances where this is done/discussed in the tests and docs), bumping the config version to v8 as result.

Notably, this doesn't change the underlying collate code itself -- there doesn't seem to be anything special there about the entity-level grouping as groups is simply passed as a list which is iterated over in a few places. Restricting the list to just ["entity_id"] means a few loops are just traversed once but doesn't result in a bunch of cruft/untouched code, and it hardly seems worth simply removing the for loops.

A couple of incidental/unrelated changes as well, I updated documentation to reflect the python 3.7+ requirement as well as the python version used by ./develop to 3.9.10.

Also, @ecsalomon -- once we're ready to merge both this and #883, we should be sure to update the upgrade-to-v8.md documentation to reflect both this change and specifying SQL files for the labels/cohort configs.

@shaycrk shaycrk merged commit 24c8019 into master Mar 28, 2022
@shaycrk shaycrk deleted the kit_no_collate_groups branch March 28, 2022 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants