Skip to content

Limit PR test matrices for adapter-only changes - #474

Open
assaftibm wants to merge 1 commit into
mainfrom
filter_pr_models
Open

Limit PR test matrices for adapter-only changes#474
assaftibm wants to merge 1 commit into
mainfrom
filter_pr_models

Conversation

@assaftibm

Copy link
Copy Markdown
Collaborator

Summary

  • generate complete model matrices before applying changed-file impact filtering
  • narrow model tests only when every changed file is an hf_adapters/hf_*.py file
  • skip model matrices for documentation-only changes and retain full matrices for mixed or unknown changes
  • keep matrix configuration shared between generation and filtering

Testing

  • .venv/bin/python -m pytest --noconftest tests/test_filter_test_matrix.py -q
  • .venv/bin/python -m ruff check .github/scripts/filter_test_matrix.py tests/test_filter_test_matrix.py
  • repository pre-commit hooks

@BenjSz

BenjSz commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

@assaftibm could you please fix the conflict?

if all(_is_documentation_path(path) for path in normalized):
return set()
if not all(_is_hf_adapter_path(path) for path in normalized):
return None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is perhaps overly conservative - as usually when someone adds an adapter they also update the documentation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Something else - we must have special treatment of hf_common.py - changes there must trigger comprehensive testing, but currently it is treated like any other adapter

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think in the case of hf_common.py almost all the adapters will be tested.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

right, but shouldn't we explicitly guarantee this?

QUESTION_ANSWERING = "question_answering_matrix"
TOKEN_CLASSIFICATION = "token_classification_matrix"
RERANKER = "reranker_matrix"
COMBINED = "combined_matrix"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is also seq_classification now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

may be solved with the conflict merge....

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it is indeed the same thing, but still can be easily missed...

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