Repository navigation
Conversation
… with feature-split matrix output
…so the fetcher does not over-select through shared mixins
…an HF token, not an accelerator
…sformer_flux.py` instead of a separate `utils.py`
| assert False, "Parameters not the same!" | ||
|
|
||
| def test_local_files_only_with_sharded_checkpoint(self): | ||
| from diffusers.models import FluxTransformer2DModel |
There was a problem hiding this comment.
Similar issue as AutoPipelines. Importing Flux and SD3 at the top of the file suggests to the test fetcher that these model objects are a dependency for anything that imports from this file.
There was a problem hiding this comment.
Based on this observation, we should update the test related sections in our agent guides, then.
There was a problem hiding this comment.
Additionally, a simple example of how this affects the dependency map would be helpful. Currently, it feels confusing.
yiyixuxu
left a comment
There was a problem hiding this comment.
thanks!, I left a comment
| return inputs | ||
|
|
||
| def test_pag_cfg(self): | ||
| from diffusers import AutoPipelineForText2Image |
There was a problem hiding this comment.
I wonder if we can make the "dependency map" more explainable and its changes visible (e.g. maybe we can keep the map as a file in the reop so changes would show up in PR diffs)
e.g. when someone writes the test_pag_sd.py for the first time in a PR (the way it was in the current main with this import at the module level), they should be able to see how it changes the dependency map and make changes accordingly - so that we can always make sure the map makes sense
Right now it's a black box, no? It's built on fresh, in-memory each time in CI and we cann't see why a given test was or wasn;t selected etc
There was a problem hiding this comment.
@yiyixuxu I don't think we can store the dependency map as a repo file because the map will probably update quite frequently. Another challenge is that PRs modifying many files, will likely trigger many tests. So a dependency map explainer might be too hard interpret in those cases.
Proposing a minimal summary section in the workflow that would run on each PR. Example here
https://github.com/huggingface/diffusers/actions/runs/37287991101
It shows how many test files were triggered by a modified file. If the PR contains modified test files (which end up triggering themselves) we just report the count instead of listing every test file in the table.
I think this should help catch weird cases. e.g. if a file outside the core modules (modeling_utils, pipeline_utils, etc.) triggers hundreds of tests, an import problem is the likely cause. We can of course store the changed dependency map as a workflow asset, and perhaps use /diffusers-bot to provide a more detailed explainer if something looks weird.
wdyt?
There was a problem hiding this comment.
@DN6
thanks, I think the approach make sense. but this particular case won't show up as anything in the current summary table though. i.e for the PR adds the test_pag_sd.py with the top level import of AutoPipelineForText2Image, I think the test file does not trigger any other test file other than itself, but it makes a lot other files rigger a lot more tests, even though these other files are not modified in the PR) - maybe we can make it more informative? e.g. we can show how many files they depends on (the difference between PR and main) etc so something like this would be visible too
There was a problem hiding this comment.
It shows how many test files were triggered by a modified file. If the PR contains modified test files (which end up triggering themselves) we just report the count instead of listing every test file in the table.
But how is this count useful to a contributor / maintainer, though? Could you provide an example?
|
/diffusers-bot review |
There was a problem hiding this comment.
🤗 Serge says:
Rewrites utils/tests_fetcher.py from regex-based to AST-based import extraction, simplifies the CI matrix to flat {name, paths, markers} entries, and moves several AutoPipeline* imports to function-level to shrink the dependency graph.
Correctness — silent test drop for class-less test files
_feature_groups_for returns [] when class_markers is empty (every selected file has only top-level functions, no ClassDef). _matrix_entries then produces zero entries for that bucket, silently dropping those tests. At least one such file exists today (tests/pipelines/ltx2/test_ltx2_diffusion_decode.py). Fix: default to ["core"] when groups is empty but paths is non-empty.
Robustness — potential KeyError in transitive closure
_merged_nested_deps indexes direct_deps[d] without a guard. If d is absent (file deletion race, conftest outside scanned trees), this raises an unhandled KeyError. The old code had an explicit check. direct_deps.get(d, []) would be safer and consistent with the "over-select rather than miss" philosophy.
serge v0.1.0 · model: claude-opus-4-6 · 21 LLM turns · 24 tool calls · 312.1s · 1094467 in / 6075 out tokens
| for d in list(direct_deps[m]): | ||
| if d.endswith("__init__.py"): | ||
| continue | ||
| new_deps = set(direct_deps[d]) - set(direct_deps[m]) |
There was a problem hiding this comment.
If d is absent from direct_deps (file deleted between glob and import resolution, or a conftest outside scanned trees), this raises an unhandled KeyError. The old code had an explicit guard.
Use .get() for robustness:
| new_deps = set(direct_deps[d]) - set(direct_deps[m]) | |
| new_deps = set(direct_deps.get(d, [])) - set(direct_deps[m]) |
… checkout has no local `main` ref
…_fetcher.py` in the container
…ecorators `tests/conftest.py` derives `NON_CORE_MARKERS` from the markers the `is_*` decorators in `testing_utils.py` apply and marks every test carrying none of them as `core` at collection time. The fetcher imports that set instead of keeping its own list and emits `-m core` for the core jobs in place of the `not (...)` complement expression. Also drop `-v` from the pytest run, which no other workflow passes.
…`; legacy mixins share their names The legacy cache and IP-Adapter mixins in `test_pipelines_common.py` are unmarked but have the same class names as the marked ones, so matching by bare name scheduled a `pipelines-cache` job whose `-m cache` run collected nothing and failed with exit code 5.
…ed test files outside the listed reach
There was a problem hiding this comment.
FYI the fetcher tends to over select tests rather than miss them.
Do we know why?
- Can we also see an action where the changeset would be based on other important features like quantization, context-parallelism? What is the workflow there?
- Is there a way to get a comparative view of the coverage with and without this PR for a given diff? If the coverage is reduced without any explanation, then that is a concern.
- Are we running the GPU tests by default? Or are we asking the maintainers to run them (through a comment)? I think the latter is better because for small and obvious changes, it can save time. Doesn't have to be this PR.
- Also, we should test the functionalities implemented, though.
Another question:
https://github.com/huggingface/diffusers/actions/runs/37564579479/job/112610663236?pr=14900 - stuff like this should be caught in the pipeline/model PRs (#14949, for example added Kandinsky6). Would the fetcher resolve this kind of problems?
| # The checkout step only marks the workspace safe in a temporary git config, so git processes the | ||
| # fetcher spawns in the container would otherwise refuse the repo as dubiously owned. | ||
| run: | | ||
| git config --global --add safe.directory "$GITHUB_WORKSPACE" |
There was a problem hiding this comment.
The fetcher is currently in workflow_dispatch. Should it have a pull_request trigger? And then we should also make sure that the fetching script is always run from the base upstream (for security reasons)?
| if [ -f test_map.json ]; then | ||
| keys=$(python3 -c 'import json; fp = open("test_map.json"); test_map = json.load(fp); fp.close(); d = list(test_map.keys()); print(json.dumps(d))') | ||
| test_map=$(python3 -c 'import json; fp = open("test_map.json"); test_map = json.load(fp); fp.close(); print(json.dumps(test_map))') | ||
| matrix=$(python3 -c 'import json; print(json.dumps(json.load(open("test_map.json"))))') |
There was a problem hiding this comment.
Is it possible to have an example matrix for quick references?
| - name: Run all selected tests on CPU | ||
| run: | | ||
| pytest -n 2 --dist=loadfile -v --make-reports=${{ matrix.modules }}_tests_cpu ${{ fromJson(needs.setup_pr_tests.outputs.test_map)[matrix.modules] }} | ||
| pytest -n 2 --dist=loadfile -m "${{ matrix.markers }}" --make-reports=${{ matrix.name }}_tests_cpu ${{ matrix.paths }} |
There was a problem hiding this comment.
Possible to include a comment to show example markers and name?
If possible, I would also increase the number of workers to 4. Doesn't hurt I guess.
| assert False, "Parameters not the same!" | ||
|
|
||
| def test_local_files_only_with_sharded_checkpoint(self): | ||
| from diffusers.models import FluxTransformer2DModel |
There was a problem hiding this comment.
Based on this observation, we should update the test related sections in our agent guides, then.
| AutoPipelineForImage2Image, | ||
| AutoPipelineForText2Image, |
There was a problem hiding this comment.
I think it's then better to replace them with their exact pipeline class so that we don't have to do the inline import. Doing it this way also preserves the expected behaviour in the dependency map? Applies elsewhere, too.
| from huggingface_hub.utils import HfHubHTTPError, is_jinja_available | ||
|
|
||
| from diffusers.models import FluxTransformer2DModel, SD3Transformer2DModel, UNet2DConditionModel | ||
| from diffusers.models import UNet2DConditionModel |
There was a problem hiding this comment.
Why is this import not being inlined then?
| assert False, "Parameters not the same!" | ||
|
|
||
| def test_local_files_only_with_sharded_checkpoint(self): | ||
| from diffusers.models import FluxTransformer2DModel |
There was a problem hiding this comment.
Additionally, a simple example of how this affects the dependency map would be helpful. Currently, it feels confusing.
| return inputs | ||
|
|
||
| def test_pag_cfg(self): | ||
| from diffusers import AutoPipelineForText2Image |
There was a problem hiding this comment.
It shows how many test files were triggered by a modified file. If the PR contains modified test files (which end up triggering themselves) we just report the count instead of listing every test file in the table.
But how is this count useful to a contributor / maintainer, though? Could you provide an example?
| warnings.simplefilter(action="ignore", category=FutureWarning) | ||
|
|
||
|
|
||
| def marker_decorators(): |
There was a problem hiding this comment.
Consider providing an example of how a minimal decorators would look like.
|
|
||
|
|
||
| @pytest.hookimpl(tryfirst=True) | ||
| def pytest_collection_modifyitems(items): |
What does this PR do?
Migrating PR CI to use the test fetcher by default. Doing this in parts to keep review manageable and testable.
First step is to just update the fetcher script.
The test fetcher determines which tests to run based on which files have been changed in a PR. It works in the following way
diffusers/srcandtests/to build a reverse dependency map. This map is a dictionary from a file in our repo to every file that would be affected if the file were changed. It is built in the following way.pyundersrc/diffusers/andtests/return the files it imports. e.g.test_pipeline_flux.py -> [src/diffusers/pipelines/flux/pipeline_flux.py,...]test_pipeline_flux.py's list also containstransformer_flux.py, attention.pyand so on, because these are modules are imported bypipeline_flux.pysrc/diffusers/pipelines/flux/pipeline_flux.py -> [tests/pipelines/flux/test_pipeline_flux.py, tests/pipelines/flux/test_pipeline_flux_control.py, ...](LoraTesterMixin → lora, MemoryTesterMixin → memory, cpu_offload, group_offload, …). It uses this to construct a feature based matrix for pipeline/models tests so that the actual tests jobs run in feature based buckets. e.g model-core, model-memory, model-attention, pipeline-core, pipeline-lora. Here is an example of a workflow running this new test matrix: https://github.com/huggingface/diffusers/actions/runs/32945379597/job/98105816662Once the fetcher script is merged into
main, we can update the relevant workflows and test them side by side with the existing CI to ensure no gaps. FYI the fetcher tends to over select tests rather than miss them.