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

Implemented a new way of recording properties that works even if test is skipped #311

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kmitrovicTT
Copy link
Contributor

@kmitrovicTT kmitrovicTT commented Mar 5, 2025

Changes

  • Added new test mark record_properties(key1=val1, key2=val2, ...) which is meant to be used as a test decorator to capture any test properties
    • Any key value pair can be used, no DB schema restrictions apply, since it is dumped as JSON/dict
    • Nevertheless, I narrowed allowed keys to ["test_category", "jax_op_name", "shlo_op_name", "model_name", "run_mode"] and implemented verification so that we don't accidentally make typos or other errors. These keys are expected by SQL in Superset.
  • If used above pytest.mark.skip (or xfail) decorator it will successfully record the data regardless, which wasn't possible with previous approach and some SQL hacks had to be done to circumvent that
  • Deprecated need for record_property and record_tt_xla_property fixtures
  • Removed start and end timestamp capturing since we have no use for them for now, and they are not trivial to capture as these other properties
  • Changed all tests to this new way of working

Result

We now have

-- Test metadata
test_case.filepath,
test.tags->>'test_name' AS test_name,
test.tags->>'specific_test_case' AS specific_test_case,
test.tags->>'model_name' AS model_name,
test.tags->>'jax_op_name' AS jax_op_name,
test.tags->>'shlo_op_name' AS shlo_op_name,
test.tags->>'test_category' AS test_category,

instead of current

-- Test metadata
test_case.filepath,
test_case.test_case_name AS test_name,
SUBSTRING(test.full_test_name FROM POSITION('::' IN test.full_test_name) + 2) AS specific_test_case,  -- strip the path down to extract test function name together with arguments, like `test_this[arg0-arg1]
-- TODO this is a workaround the fact that skipped models don't record model_name property. We derive it from test_case_name instead in those cases
CASE
    WHEN test_case.op_name IS NOT NULL
    THEN NULL  -- If op_name is not NULL, set model_name to NULL
    WHEN test_case.model_name IS NOT NULL
    THEN test_case.model_name  -- If model_name is not NULL, use model_name as it is
    WHEN test_case.test_case_name ~ '^[^_]+_(.*)_.*$'
    THEN REGEXP_REPLACE(test_case.test_case_name, '^[^_]+_(.*)_.*$', '\1')
    ELSE NULL  -- If none of the above conditions match, set model_name to NULL
END AS model_name,
test_case.framework_op_name,
test_case.op_name,
-- Test category
CASE
    WHEN test_case.filepath LIKE 'tests/jax/ops%' THEN 'op_test'
    WHEN test_case.filepath LIKE 'tests/jax/graphs%' THEN 'graph_test'
    WHEN test_case.filepath LIKE 'tests/jax/models%' THEN 'model_test'
    ELSE 'other'  -- If test doesn't fall into one of the above categories, like test_scalar_dtype, test_ranks, etc.
END AS test_category,

Note

Once this enters main, dashboard might be broken until I switch to new queries.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.97%. Comparing base (c30bbf0) to head (82bc745).
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #311   +/-   ##
=======================================
  Coverage   77.97%   77.97%           
=======================================
  Files          21       21           
  Lines         990      990           
=======================================
  Hits          772      772           
  Misses        218      218           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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