fix: preserve alias in metric tests for all generic test helpers - #1908
Open
CTWalk wants to merge 1 commit into
Open
fix: preserve alias in metric tests for all generic test helpers#1908CTWalk wants to merge 1 commit into
CTWalk wants to merge 1 commit into
Conversation
The generic test helpers accept alias=, but only gte() forwards it into
the metric-level test. eq(), not_eq(), gt(), lt(), lte(), is_in(), and
not_in() forward it only to DescriptorTest, so an alias configured on a
Report metric test was silently dropped and the result description fell
back to the metric display name.
MetricTest.alias is the value used to build that description in
core/metric_types.py ("{self.alias or value.display_name}: ..."), so for
seven of the eight helpers the field could not be set through the public
helper at all.
Pass the already accepted alias into the seven metric constructors.
alias defaults to None, so behavior is unchanged for callers that do not
pass it. gte() is untouched and serves as the reference.
Add a parameterized regression over all eight helpers, asserting the
alias survives on both the metric and the descriptor.
Fixes evidentlyai#1907
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for maintaining Evidently. This is a small consistency fix in the
generic test helpers; the reproduction and reasoning are below.
Related issue: #1907
What
gte()forwardsalias=into its metric test. The seven sibling helpers(
eq,not_eq,gt,lt,lte,is_in,not_in) also acceptalias=,but forward it only to
DescriptorTest. When one of them is used as ametric-level test in a
Report, the configured alias is silently dropped.Why it matters
MetricTest.aliasis the custom name used to build the test description:For seven of the eight helpers that field can never be set through the public
helper, so a name the user configured never reaches the report.
Reproduce
On
main(a4aa4c2, 0.7.21):Before:
After:
Change
src/evidently/tests/aliases.py— pass the already acceptedaliasargument into the metric-test constructor in the seven helpers.
gteisuntouched and serves as the reference behavior.
ruff formatrewraps fiveof those calls onto multiple lines; the only semantic change is
alias=alias.tests/future/tests/test_generic_tests_creation.py— one parameterizedregression over all eight helpers, asserting the alias survives on both the
metric and the descriptor.
No new API, parameter, state, or dependency.
aliasdefaults toNone, sobehavior is unchanged for every caller that does not pass it.
Checks
Run locally against this branch on Python 3.12:
pytest tests/future/tests/test_generic_tests_creation.py—7 failed, 8 passedbefore the source change,15 passedafter. Reverting only thesource change brings the seven failures back, so the new test does pin the
behavior.
tests/future—1626 passed, 6 skipped, no failures.ruff checkandruff format --checkon both files, using the repository'sruff.tomland pinnedruff==0.3.7.Every other
alias=usage in the test tree is descriptor-level, so noexisting test exercises the changed metric path.
Not run locally:
tests/future/test_ui(needssqlalchemy), the legacytests/tree, and mypy.One open question
The
@overloadsignatures inevidently/tests/aliases.pycurrently declarealiasonly on theDescriptorTest-returning forms, so a typedgt(1, alias="quality gate")resolves to the descriptor overload. This PRdeliberately leaves the overloads alone — the same gap already applies to
gte(), so nothing gets worse — but if you would like the metric-shapedoverloads to declare
aliasas well, I am happy to extend this PR.