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

Feat: allow marking audits as [non-]blocking at use site #2813

Merged
merged 3 commits into from
Jun 25, 2024

Conversation

georgesittas
Copy link
Contributor

@georgesittas georgesittas commented Jun 24, 2024

Until now, audits could be marked as blocking (default) or non-blocking only at their definition. Some users reasonably found this counter-intuitive, since the audit's logic and whether it blocks execution are orthogonal concepts.

This PR adds support for marking an audit as blocking or non-blocking at its use-site, i.e. in a model that references it. The syntax to do that is demonstrated below:

MODEL (
  name model_name,
  kind FULL,
  audits (
    some_audit(blocking := false)
  )
);

...

@georgesittas georgesittas requested a review from a team June 24, 2024 19:31
@georgesittas
Copy link
Contributor Author

FYI @plaflamme

@izeigerman
Copy link
Member

Can we please update documentation

@@ -831,6 +831,10 @@ def _audit(
skipped=True,
)

# Model's "blocking" argument takes precedence over the audit's default setting
blocking = audit_args.pop("blocking", None)
blocking = blocking == exp.true() if blocking else audit.blocking
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder if there's a better way to detect truthy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I was looking for a better way too, but didn't find something.. I thought of overriding __eq__ for Boolean to check for True / False but didn't like the idea much. Open to suggestions.

Copy link
Member

@izeigerman izeigerman Jun 25, 2024

Choose a reason for hiding this comment

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

Does this mean we can't pass a result of a macro in here?

Copy link
Member

Choose a reason for hiding this comment

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

Or I guess it will be evaluated during rendering? Can we have a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me take a look, I didn't think much about this use case but it makes sense.

Copy link
Contributor Author

@georgesittas georgesittas Jun 25, 2024

Choose a reason for hiding this comment

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

Done - check out 15d6cab. Also tested in a dummy project, seems to be working fine.

@georgesittas
Copy link
Contributor Author

Can we please update documentation

Done, thanks for pointing it out 👍

@georgesittas georgesittas merged commit 4c7b769 into main Jun 25, 2024
15 checks passed
@georgesittas georgesittas deleted the jo/set_audit_blocking_inline branch June 25, 2024 16:32
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