Allow rules to be skipped #66

diorge · 2024-06-27T19:58:33Z

Solves #48 (partially?)

Introduce ModelFilter, which allows Rule objects to be skipped, by filtering on a Model. Adding filters is done in a similar way as rules, and can be configured using the pyproject.toml configuration.

Rules can now return an empty value SkipRule(). When doing so, the rule is not computed at all by the Scorer.

matthieucan

Thank you for your contribution! 🙏

Left a comment for discussion on the overall design of the feature.

docs/create_rules.md

diorge · 2024-07-11T18:18:04Z

I'm currently working on updating this with the Skip feature as well. Just externalizing here that having parametrized skips work well enough as long as they are used programatically and not through the configfile interface. I thought about having generic skips such as "skip_schema(schema)" and "keep_schema(schema)", but that would not work nicely with the configfile.

On that note, Skip is not a great name because inverting it is also a valid use case. Any other name recommendation? RuleFilter?

jochemvandooren · 2024-07-12T11:57:25Z

I'm currently working on updating this with the Skip feature as well. Just externalizing here that having parametrized skips work well enough as long as they are used programatically and not through the configfile interface. I thought about having generic skips such as "skip_schema(schema)" and "keep_schema(schema)", but that would not work nicely with the configfile.

On that note, Skip is not a great name because inverting it is also a valid use case. Any other name recommendation? RuleFilter?

Great to hear @diorge! Yes I think it will be hard to configure these properly with the config files, let's keep it simple (for now)!

Could you maybe elaborate on "inverting it"? That's not entirely clear to me. Anyway: RuleFilter is also a good name to be honest, or maybe ModelFilter, as it's actually filtering out some models

diorge · 2024-07-12T14:25:41Z

Could you maybe elaborate on "inverting it"?

Both "apply only on schema X" and "apply everywhere except schema X" are valid filters. One is skipping X, and the other is the invert operation.

Anyway, yeah, ModelFilter is a much better name for the feature, thanks for the input.

Changes due to prettifier, ruff and mypy

- Filters no longer have their own config - Rules take instances of filters instead of types - Rule configuration now allows `model_filter_name`

jochemvandooren

Amazing work 🚀 It works super well and code looks good. I have left a couple of suggestions and questions. Will take another look later this week!

src/dbt_score/model_filter.py

src/dbt_score/rule.py

docs/create_rules.md

Use a new field rule.model_filter_names instead of using rule.config["model_filter_names"]

diorge · 2024-07-30T14:54:34Z

My bad on not having installed the pre-commit hooks, the linter should be passing now

druzhinin-kirill · 2024-07-31T15:08:17Z

Hey @diorge !

Thanks for this work, a really great feature 💪

I see now there are two mechanisms to disable evaluation for a model-rule pair. Is there any practical difference between them so we should keep both? For me, it feels like ModelFilter provides all the functionality to achieve skipping. It also keep rule logic abstracted and seems more natural (verify if the rule should be applied before applying). Do you think the SkipRule() mechanism needs to be kept?

diorge · 2024-07-31T16:01:25Z

@druzhinin-kirill this has been brought up in a conversation marked as resolved. SkipRule could be removed, but makes some use cases easier to deal with. I'd argue for most use cases, having a simple return SkipRule() is simpler than creating a ModelFilter, and filters won't be reusable by much.

I'm not too strong for keeping it, but I do think it's a nice addition that does not bloat the lib too much.

druzhinin-kirill · 2024-08-01T07:41:25Z

@druzhinin-kirill this has been brought up in a conversation marked as resolved. SkipRule could be removed, but makes some use cases easier to deal with. I'd argue for most use cases, having a simple return SkipRule() is simpler than creating a ModelFilter, and filters won't be reusable by much.

I'm not too strong for keeping it, but I do think it's a nice addition that does not bloat the lib too much.

I see, got your points 👍
However, IMO the rule logic should be separated from the filter. Rules should left untouched if filters change. ModelFilter seems pretty powerful and clear from the design perspective, so following

There should be one-- and preferably only one --obvious way to do it.

I would vote to keep filters only. They might have a little overhead compared to SkipRule in specific cases indeed, but still straightforward enough to understand and implement 💡

WDYT @jochemvandooren @matthieucan ?

diorge · 2024-08-02T13:47:40Z

I'd compare the relationship of SkipRule and ModelFilter to that of tuple and list: they do the same thing in the end, but the use cases are different enough.

Anyway, if it is decided to remove the feature, I'd appreciate it if anyone else could pick this PR. I won't be able to work on it in the following weeks.

matthieucan · 2024-08-07T14:57:41Z

@druzhinin-kirill this has been brought up in a conversation marked as resolved. SkipRule could be removed, but makes some use cases easier to deal with. I'd argue for most use cases, having a simple return SkipRule() is simpler than creating a ModelFilter, and filters won't be reusable by much.
I'm not too strong for keeping it, but I do think it's a nice addition that does not bloat the lib too much.

I see, got your points 👍 However, IMO the rule logic should be separated from the filter. Rules should left untouched if filters change. ModelFilter seems pretty powerful and clear from the design perspective, so following
There should be one-- and preferably only one --obvious way to do it.
I would vote to keep filters only. They might have a little overhead compared to SkipRule in specific cases indeed, but still straightforward enough to understand and implement 💡

WDYT @jochemvandooren @matthieucan ?

I agree with that sentiment. Separating rules (pure logic) and configuration (to whom does that logic apply, with which parameters) will keep things simple, and avoid some edge-cases that do not warrant the extra complexity IMO.

matthieucan · 2024-08-07T15:02:53Z

docs/create_rules.md

+class SkipSchemaY(ModelFilter):
+    description = "Applies a rule to every schema but Y."
+    def evaluate(self, model: Model) -> bool:
+      return model.schema.lower() != 'y'


Do we know use-cases where the class version (as opposed to the decorator version) would be necessary?

Not yet, but no harm in having it as it does not really change implementation and it follows the same design of @rule.

jochemvandooren · 2024-08-14T13:41:48Z

@diorge I made some changes to the PR! Mainly got rid of the SkipRule, everything else is the same. @druzhinin-kirill @matthieucan Please have another look so we can get this one merged 🙌

druzhinin-kirill · 2024-08-15T07:06:08Z

src/dbt_score/evaluation.py

@@ -57,11 +57,11 @@ def evaluate(self) -> None:
            self.results[model] = {}
            for rule in rules:
                try:
-                    result: RuleViolation | None = rule.evaluate(model, **rule.config)
+                    if rule.should_evaluate(model):  #  Consider model filter(s).


IIUC, if a filter skips the model, it's not now visible to the user. In the case of complex filters, it might be important to flag skipping so the filter behaves expectedly

It might be interesting to either:

emit a warning if the model skipped (maybe too much noise)

Keep track of skipped models in the skipped list attr. In the end, Formatter can use this to print at least the number of skipped models.

My proposal is to add self.skips and track Model, Filter

I am fine to outline it as a new feature request.

Yes let's keep it for another PR, then we can already release this one 🚀

druzhinin-kirill

Great work @diorge! Thanks a lot for bringing life 🚀

jochemvandooren · 2024-08-15T10:22:22Z

@diorge Thanks a lot for your contribution! 🙌

diorge added 2 commits June 27, 2024 16:43

Allow rules to be skipped (PicnicSupermarket#48)

8ec7e73

Rules can now return an empty value SkipRule(). When doing so, the rule is not computed at all by the Scorer.

Docs/example for skipping

40d50d9

matthieucan reviewed Jun 28, 2024

View reviewed changes

docs/create_rules.md Outdated Show resolved Hide resolved

diorge added 8 commits July 12, 2024 11:45

Basic interface for ModelFilter/skip

3414556

Rules now support Model Filters

a5668a6

Make filters into frozen set, ran linter

9b47413

Changes due to prettifier, ruff and mypy

Extra test for model filter

4b22e25

Rules now support ModelFilter

d17d0a4

- Filters no longer have their own config - Rules take instances of filters instead of types - Rule configuration now allows `model_filter_name`

Docs for new Filter feature

3ab2d5c

Move filter-popping to load function

b5ff89b

Style: Linting/auto-fix

fc972e3

diorge requested review from matthieucan and jochemvandooren July 16, 2024 19:35

jochemvandooren reviewed Jul 23, 2024

View reviewed changes

src/dbt_score/model_filter.py Show resolved Hide resolved

src/dbt_score/model_filter.py Outdated Show resolved Hide resolved

src/dbt_score/rule.py Outdated Show resolved Hide resolved

docs/create_rules.md Outdated Show resolved Hide resolved

diorge added 4 commits July 23, 2024 14:48

docs(skip): rm last usage of "Skip" class name

5c1ad44

feat(skip): mv filter names from config{} to field

a974ccd

Use a new field rule.model_filter_names instead of using rule.config["model_filter_names"]

docs(skip): separate docs for skip and filters

255acce

style(lint): auto prettifier

9841366

matthieucan reviewed Aug 7, 2024

View reviewed changes

Add documentation

8f9b3ff

jochemvandooren requested a review from matthieucan August 14, 2024 13:39

jochemvandooren requested review from jochemvandooren and druzhinin-kirill August 14, 2024 13:39

Add changelog entry

9c32398

druzhinin-kirill reviewed Aug 15, 2024

View reviewed changes

jochemvandooren requested a review from druzhinin-kirill August 15, 2024 10:14

druzhinin-kirill approved these changes Aug 15, 2024

View reviewed changes

Merge branch 'master' into dbrognara/feat/skip-rule

38f2e84

jochemvandooren merged commit af26d7b into PicnicSupermarket:master Aug 15, 2024
3 checks passed

jochemvandooren mentioned this pull request Oct 1, 2024

Enable/disable rules per schema #48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow rules to be skipped #66

Allow rules to be skipped #66

diorge commented Jun 27, 2024 •

edited by jochemvandooren

Loading

matthieucan left a comment

diorge commented Jul 11, 2024

jochemvandooren commented Jul 12, 2024

diorge commented Jul 12, 2024

jochemvandooren left a comment

diorge commented Jul 30, 2024

druzhinin-kirill commented Jul 31, 2024

diorge commented Jul 31, 2024

druzhinin-kirill commented Aug 1, 2024

diorge commented Aug 2, 2024

matthieucan commented Aug 7, 2024

matthieucan Aug 7, 2024

jochemvandooren Aug 14, 2024

jochemvandooren commented Aug 14, 2024

druzhinin-kirill Aug 15, 2024

jochemvandooren Aug 15, 2024

druzhinin-kirill left a comment

jochemvandooren commented Aug 15, 2024

Allow rules to be skipped #66

Allow rules to be skipped #66

Conversation

diorge commented Jun 27, 2024 • edited by jochemvandooren Loading

matthieucan left a comment

Choose a reason for hiding this comment

diorge commented Jul 11, 2024

jochemvandooren commented Jul 12, 2024

diorge commented Jul 12, 2024

jochemvandooren left a comment

Choose a reason for hiding this comment

diorge commented Jul 30, 2024

druzhinin-kirill commented Jul 31, 2024

diorge commented Jul 31, 2024

druzhinin-kirill commented Aug 1, 2024

diorge commented Aug 2, 2024

matthieucan commented Aug 7, 2024

matthieucan Aug 7, 2024

Choose a reason for hiding this comment

jochemvandooren Aug 14, 2024

Choose a reason for hiding this comment

jochemvandooren commented Aug 14, 2024

druzhinin-kirill Aug 15, 2024

Choose a reason for hiding this comment

jochemvandooren Aug 15, 2024

Choose a reason for hiding this comment

druzhinin-kirill left a comment

Choose a reason for hiding this comment

jochemvandooren commented Aug 15, 2024

diorge commented Jun 27, 2024 •

edited by jochemvandooren

Loading