Skip to content

[ci breaking-change] feat: discrete metrics as default#3067

Merged
mikewilli merged 5 commits intomainfrom
2704-discrete-as-default
Mar 30, 2026
Merged

[ci breaking-change] feat: discrete metrics as default#3067
mikewilli merged 5 commits intomainfrom
2704-discrete-as-default

Conversation

@mikewilli
Copy link
Copy Markdown
Contributor

@mikewilli mikewilli commented Mar 26, 2026

  • set default value for discrete metrics option as True
  • use --no-discrete-metrics for ongoing experiments
  • move the discrete_metrics argo parameter to the experiment instead of workflow level (so that it can vary by experiment as needed)

Edited to add:

  • don't add warning message if the PR title already starts with [ci breaking-change]
  • updates tests for the special case discrete logic

Also clarifying that the DISCRETE_AS_DEFAULT_THRESHOLD date is set to 2026-03-31 because I will plan to land this on 2026-03-30, which means the first run with discrete-as-default will be on 2026-03-31, and anything computed before that will used non-discrete logic.

@github-actions
Copy link
Copy Markdown

⚠️ Detected changes to the Argo workflow, which could indicate breaking changes.

Do you want to mark this image with the breaking tag using [ci breaking-change]?

@github-actions
Copy link
Copy Markdown

⚠️ Detected changes to the Argo workflow, which could indicate breaking changes.

Do you want to mark this image with the breaking tag using [ci breaking-change]?

@mikewilli mikewilli changed the title feat: discrete metrics as default [ci breaking-change] feat: discrete metrics as default Mar 26, 2026
Copy link
Copy Markdown
Collaborator

@scholtzan scholtzan left a comment

Choose a reason for hiding this comment

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

Some mypy issues, but otherwise lgtm

Comment on lines +145 to +155
# SPECIAL CASE DISCRETE METRICS LOGIC
# if the experiment's versioning date is before we flipped to discrete metrics as default
# then don't pass the --discrete-metrics flag
# unless --discrete-metrics was passed as an explicit command-line parameter
client = BigQueryClient(self.project_id, self.dataset_id)
DISCRETE_AS_DEFAULT_THRESHOLD = datetime(2026, 3, 26, tzinfo=pytz.UTC)
discrete_source = click.get_current_context().get_parameter_source("discrete_metrics")
explicit_discrete = discrete_source == ParameterSource.COMMANDLINE
discrete_metrics = (
"--discrete-metrics" if self.discrete_metrics else "--no-discrete-metrics"
)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should we create a ticket somewhere to remove this in a couple of months?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure I can make a ticket. Not sure when we'll want to remove it, but a few months should be enough for everything but some holdbacks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@github-actions
Copy link
Copy Markdown

⚠️ Detected changes to the Argo workflow, which could indicate breaking changes.

Do you want to mark this image with the breaking tag using [ci breaking-change]?

@mikewilli mikewilli added this pull request to the merge queue Mar 30, 2026
Merged via the queue into main with commit 4b6919d Mar 30, 2026
5 checks passed
@mikewilli mikewilli deleted the 2704-discrete-as-default branch March 30, 2026 21:18
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.

2 participants