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

Refactor config #214

Merged
merged 30 commits into from
Jul 4, 2022
Merged

Refactor config #214

merged 30 commits into from
Jul 4, 2022

Conversation

sbrugman
Copy link
Collaborator

@sbrugman sbrugman commented Jun 2, 2022 •
edited
Loading

Preparations for v1.0.0 release

Config Refactor (breaking)

Structured config using pydantic.

  • Hierarchical configuration in config.py provides context to options
  • This setup prevents code duplication (functions with many arguments, various places to set defaults, docstrings...)
  • Introducing new parameters is simplified
  • Documentation is updated to reflect the new syntax
  • Validation is handled by the configuration, e.g. fewer type check in the code

Profiles/Comparison Registry refactor (breaking)

The Profile and Comparison registries are extended to generalize to other existing implementations (chi2, ks, etc.)
The definitions are moved from popmon/stats/numpy.py to popmon/analysis/(profiles|comparisons)

Enhancements

  • String representation for Pipeline and Module
  • Maintain section position when changing features
  • Guarantee the ordering of metrics in traffic lights overview

Removed

Removal of unused functionality:

  • "worst" traffic light (unused, trivial to compute post-hoc)
  • Plotting of individual alerts and traffic lights (this is replaced by the overview tables)
  • docs: Removed outdated tree, does not provide any additional information over what's available on github

Internal improvements / fixes

  • top argument for plot_heatmap_b64 is now supported
  • Replace eval with lambda functions
  • Metric pipelines are now constructed of building blocks (functions) to prevent code duplication
  • Make test robust against floating point error
  • Enable docs api structure and jupyter notebook qa in ci/cd

Base automatically changed from develop to master June 14, 2022 09:44
@sbrugman sbrugman changed the base branch from master to develop June 14, 2022 11:42
This was linked to issues Jun 15, 2022
Turn global config into local configuration #163
Closed
Refactor: remove code duplication #165
Closed
@sbrugman sbrugman removed a link to an issue Jun 15, 2022
Refactor: remove code duplication #165
Closed
@sbrugman sbrugman force-pushed the refactor-config branch 4 times, most recently from 1c8b0d4 to f05d8ce Compare June 15, 2022 15:30
@sbrugman sbrugman marked this pull request as ready for review June 15, 2022 15:30
@sbrugman sbrugman requested review from pradyot-09, tomcis and mbaak and removed request for pradyot-09, tomcis and mbaak June 21, 2022 13:24
sbrugman
sbrugman commented Jun 23, 2022
View reviewed changes
popmon/config.py Outdated Show resolved Hide resolved
sbrugman added 2 commits June 27, 2022 16:50
@sbrugman
refactor: replace eval with callable
e759397
@sbrugman
refactor: remove worst traffic light
10f2abd 
BREAKING CHANGE: the `worst` entry will no long be present in the datastore
sbrugman added 18 commits June 28, 2022 16:20
@sbrugman
feat(config): structured config using pydantic
9ea128b 
Hierarchical configuration in config.py provides context to options
This setup prevents code duplication (functions with many arguments, various
places to set defaults, docstrings...)
Introducing new parameters is simplified
Documentation is updated to reflect the new syntax

BREAKING CHANGE: new configuration syntax
@sbrugman
feat(registry): add ks, pearson, chi2 to registry
2611440 
Allow for changing parameters of comparison functions
@sbrugman
refactor(config): update configuration in pipelines
aa26e91
@sbrugman
feat(registry): generalize registry
81dd7b8 
Generalized the registry pattern from profiles
and comparisons to a single class
@sbrugman
test(registry): profiles and comparisons
c0e8d0f
@sbrugman
test(config): update configuration in tests
5cb5fad
@sbrugman
chore: update notebooks
bc7402c
@sbrugman
chore: update example syntax
d46e805
@sbrugman
chore: remove outdated and redundant tree
cda0315
@sbrugman
docs(config): update configuration examples
5a6cb2f
@sbrugman
docs(registry): instructions on comparison parameter setting
2b01a7f
@sbrugman
docs(config): configuration parameter docstrings
ea7bd9f
@sbrugman
docs: update api documentation
8ed0c60
@sbrugman
feat(config): settings required parameter
e6acf28
@sbrugman
fix: guaranteed ordering of traffic light metrics
3c6ce5e 
Based on lexicographical ordering
@sbrugman
feat: keep section when changing features
9dc4e6e
@sbrugman
feat: string representation for base classes
5e369ff
@sbrugman
test: string representation base classes
1532239
mbaak
mbaak approved these changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@mbaak mbaak left a comment

Choose a reason for hiding this comment

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

Wow, a lot of work went into this.
I have just one comment about the usage of kwargs in metrics.py. With the settings object we should probably get rid of that that, it's a bit confusing now. But it can be done at a later time afaic.

popmon/pipeline/metrics.py
window=10,
shift=1,
monitoring_rules=None,
pull_rules=None,
features=None,
**kwargs,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey Simon, just one comment.
We should probably take away the kwargs argument here.
This can also be fixed later, but right now it's a bit confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Is addressed in the follow-up https://github.com/ing-bank/popmon/pulls/229

@sbrugman sbrugman merged commit 77fab29 into develop Jul 4, 2022
@sbrugman sbrugman deleted the refactor-config branch July 4, 2022 12:42
@sbrugman sbrugman restored the refactor-config branch July 4, 2022 12:42
@sbrugman sbrugman deleted the refactor-config branch July 4, 2022 12:42
@sbrugman sbrugman mentioned this pull request Jul 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pradyot-09 pradyot-09 pradyot-09 approved these changes

@mbaak mbaak mbaak approved these changes

@tomcis tomcis Awaiting requested review from tomcis

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

Successfully merging this pull request may close these issues.

Turn global config into local configuration
3 participants
@sbrugman @mbaak @pradyot-09