Repository navigation
[CI] Update test fetcher script #14607
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
base: main
Are you sure you want to change the base?
Changes from all commits
341e55f
c7c8cc4
4242566
bd8d8b8
b19052c
7e674e0
dbc8d8a
51e0728
e258130
a8b2c8e
a5af804
bc5488a
bcfeaa9
7e67c09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,6 @@ jobs: | |
| shell: bash | ||
| outputs: | ||
| matrix: ${{ steps.set_matrix.outputs.matrix }} | ||
| test_map: ${{ steps.set_matrix.outputs.test_map }} | ||
| steps: | ||
| - name: Checkout diffusers | ||
| uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0 | ||
|
|
@@ -44,42 +43,45 @@ jobs: | |
| diffusers-cli env | ||
| echo $(git --version) | ||
| - name: Fetch Tests | ||
| # 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" | ||
| python utils/tests_fetcher.py | tee test_preparation.txt | ||
| - name: Report selection in job summary | ||
| run: | | ||
| if [ -f test_fetcher_summary.md ]; then | ||
| cat test_fetcher_summary.md >> $GITHUB_STEP_SUMMARY | ||
| fi | ||
| - name: Report fetched tests | ||
| uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6.0.0 | ||
| with: | ||
| name: test_fetched | ||
| path: test_preparation.txt | ||
| - id: set_matrix | ||
| name: Create Test Matrix | ||
| # The `keys` is used as GitHub actions matrix for jobs, i.e. `models`, `pipelines`, etc. | ||
| # The `test_map` is used to get the actual identified test files under each key. | ||
| # If no test to run (so no `test_map.json` file), create a dummy map (empty matrix will fail) | ||
| # `test_map.json` is a list of matrix entries `{"name", "paths", "markers"}`, one job each. | ||
| # If no test to run (so no `test_map.json` file), emit an empty list and skip the test job. | ||
| run: | | ||
| 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"))))') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to have an example matrix for quick references? |
||
| else | ||
| keys=$(python3 -c 'keys = ["dummy"]; print(keys)') | ||
| test_map=$(python3 -c 'test_map = {"dummy": []}; print(test_map)') | ||
| matrix='[]' | ||
| fi | ||
| echo $keys | ||
| echo $test_map | ||
| echo "matrix=$keys" >> $GITHUB_OUTPUT | ||
| echo "test_map=$test_map" >> $GITHUB_OUTPUT | ||
| echo "matrix=$matrix" >> $GITHUB_OUTPUT | ||
| echo "Matrix: $matrix" | ||
|
|
||
| run_pr_tests: | ||
| permissions: | ||
| contents: read | ||
| name: Run PR Tests | ||
| name: ${{ matrix.name }} | ||
| needs: setup_pr_tests | ||
| if: contains(fromJson(needs.setup_pr_tests.outputs.matrix), 'dummy') != true | ||
| if: needs.setup_pr_tests.outputs.matrix != '[]' | ||
| strategy: | ||
| fail-fast: false | ||
| max-parallel: 2 | ||
| matrix: | ||
| modules: ${{ fromJson(needs.setup_pr_tests.outputs.matrix) }} | ||
| include: ${{ fromJson(needs.setup_pr_tests.outputs.matrix) }} | ||
| runs-on: | ||
| group: aws-general-8-plus | ||
| container: | ||
|
|
@@ -105,20 +107,20 @@ jobs: | |
|
|
||
| - 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 }} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| - name: Failure short reports | ||
| if: ${{ failure() }} | ||
| continue-on-error: true | ||
| run: | | ||
| cat reports/${{ matrix.modules }}_tests_cpu_stats.txt | ||
| cat reports/${{ matrix.modules }}_tests_cpu_failures_short.txt | ||
| cat reports/${{ matrix.name }}_tests_cpu_stats.txt | ||
| cat reports/${{ matrix.name }}_tests_cpu_failures_short.txt | ||
|
|
||
| - name: Test suite reports artifacts | ||
| if: ${{ always() }} | ||
| uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6.0.0 | ||
| with: | ||
| name: ${{ matrix.modules }}_test_reports | ||
| name: ${{ matrix.name }}_test_reports | ||
| path: reports | ||
|
|
||
| run_staging_tests: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,10 +15,13 @@ | |
| # tests directory-specific settings - this file is run automatically | ||
| # by pytest before any tests are run | ||
|
|
||
| import ast | ||
| import sys | ||
| import warnings | ||
| from os.path import abspath, dirname, join | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| # allow having multiple repository checkouts and not needing to remember to rerun | ||
| # 'pip install -e .[dev]' when switching between checkouts and running tests. | ||
|
|
@@ -30,7 +33,37 @@ | |
| warnings.simplefilter(action="ignore", category=FutureWarning) | ||
|
|
||
|
|
||
| def marker_decorators(): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider providing an example of how a minimal |
||
| """Map each `is_*` decorator in testing_utils.py to the pytest marker it applies. | ||
|
|
||
| Read from the source with `ast` rather than imported, so `utils/tests_fetcher.py` can call this without | ||
| importing torch. Matches `def is_x(test_case): return pytest.mark.<marker>(test_case)`. | ||
| """ | ||
| tree = ast.parse(open(join(dirname(__file__), "testing_utils.py"), encoding="utf-8").read()) | ||
| decorators = {} | ||
| for node in ast.walk(tree): | ||
| if not isinstance(node, ast.FunctionDef) or not node.name.startswith("is_"): | ||
| continue | ||
| for ret in ast.walk(node): | ||
| if isinstance(ret, ast.Return) and isinstance(ret.value, ast.Call): | ||
| func = ret.value.func | ||
| if ( | ||
| isinstance(func, ast.Attribute) | ||
| and isinstance(func.value, ast.Attribute) | ||
| and func.value.attr == "mark" | ||
| ): | ||
| decorators[node.name] = func.attr | ||
| return decorators | ||
|
|
||
|
|
||
| # Every marker an `is_*` decorator applies takes a test out of the `core` set. CI runs each feature in its | ||
| # own job (`-m lora`, ...) and everything else as `-m core`, so a test carrying none of these is marked | ||
| # `core` at collection time. `utils/tests_fetcher.py` uses the same set to decide which jobs to schedule. | ||
| NON_CORE_MARKERS = set(marker_decorators().values()) | ||
|
|
||
|
|
||
| def pytest_configure(config): | ||
| config.addinivalue_line("markers", "core: marks tests carrying none of the feature markers (added automatically)") | ||
| config.addinivalue_line("markers", "big_accelerator: marks tests as requiring big accelerator resources") | ||
| config.addinivalue_line("markers", "lora: marks tests for LoRA/PEFT functionality") | ||
| config.addinivalue_line("markers", "ip_adapter: marks tests for IP Adapter functionality") | ||
|
|
@@ -53,6 +86,14 @@ def pytest_configure(config): | |
| config.addinivalue_line("markers", "nightly: mark test as nightly") | ||
|
|
||
|
|
||
| @pytest.hookimpl(tryfirst=True) | ||
| def pytest_collection_modifyitems(items): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this for? |
||
| """Mark every test with no feature marker as `core`, before `-m` deselection runs.""" | ||
| for item in items: | ||
| if not any(m.name in NON_CORE_MARKERS for m in item.iter_markers()): | ||
| item.add_marker("core") | ||
|
|
||
|
|
||
| def pytest_addoption(parser): | ||
| from .testing_utils import pytest_addoption_shared | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ | |
| from huggingface_hub import ModelCard, delete_repo, snapshot_download, try_to_load_from_cache | ||
| from huggingface_hub.utils import HfHubHTTPError, is_jinja_available | ||
|
|
||
| from diffusers.models import FluxTransformer2DModel, SD3Transformer2DModel, UNet2DConditionModel | ||
| from diffusers.models import UNet2DConditionModel | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this import not being inlined then? |
||
|
|
||
| from ..others.test_utils import TOKEN, USER, is_staging_test | ||
| from ..testing_utils import ( | ||
|
|
@@ -118,6 +118,8 @@ def test_cached_files_are_used_when_no_internet(self): | |
| assert False, "Parameters not the same!" | ||
|
|
||
| def test_local_files_only_with_sharded_checkpoint(self): | ||
| from diffusers.models import FluxTransformer2DModel | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on this observation, we should update the test related sections in our agent guides, then.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Additionally, a simple example of how this affects the dependency map would be helpful. Currently, it feels confusing. |
||
|
|
||
| repo_id = "hf-internal-testing/tiny-flux-sharded" | ||
| error_response = mock.Mock(status_code=500, headers={}, json=mock.Mock(return_value={})) | ||
| # `resolve_revision` inspects `error.response.status_code` to tell a Hub outage from a definitive answer, | ||
|
|
@@ -230,6 +232,8 @@ def test_keep_modules_in_fp32(self): | |
| A simple tests to check if the modules under `_keep_in_fp32_modules` are kept in fp32 when we load the model in fp16/bf16 | ||
| Also ensures if inference works. | ||
| """ | ||
| from diffusers.models import SD3Transformer2DModel | ||
|
|
||
| fp32_modules = SD3Transformer2DModel._keep_in_fp32_modules | ||
|
|
||
| for torch_dtype in [torch.bfloat16, torch.float16]: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,8 +22,6 @@ | |
| from transformers import AutoConfig, AutoTokenizer, T5EncoderModel | ||
|
|
||
| from diffusers import ( | ||
| AutoPipelineForImage2Image, | ||
| AutoPipelineForText2Image, | ||
|
Comment on lines
-25
to
-26
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| Kandinsky3Pipeline, | ||
| Kandinsky3UNet, | ||
| VQModel, | ||
|
|
@@ -177,6 +175,8 @@ def cleanup(self): | |
| backend_empty_cache(torch_device) | ||
|
|
||
| def test_kandinskyV3(self): | ||
| from diffusers import AutoPipelineForText2Image | ||
|
|
||
| pipe = AutoPipelineForText2Image.from_pretrained( | ||
| "kandinsky-community/kandinsky-3", variant="fp16", torch_dtype=torch.float16 | ||
| ) | ||
|
|
@@ -203,6 +203,8 @@ def test_kandinskyV3(self): | |
| assert np.allclose(image_np, expected_image_np, atol=5e-2) | ||
|
|
||
| def test_kandinskyV3_img2img(self): | ||
| from diffusers import AutoPipelineForImage2Image | ||
|
|
||
| pipe = AutoPipelineForImage2Image.from_pretrained( | ||
| "kandinsky-community/kandinsky-3", variant="fp16", torch_dtype=torch.float16 | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,6 @@ | |
|
|
||
| from diffusers import ( | ||
| AutoencoderKL, | ||
| AutoPipelineForText2Image, | ||
| DDIMScheduler, | ||
| StableDiffusionPAGPipeline, | ||
| StableDiffusionPipeline, | ||
|
|
@@ -253,6 +252,8 @@ def get_inputs(self, device, generator_device="cpu", seed=1, guidance_scale=7.0) | |
| return inputs | ||
|
|
||
| def test_pag_cfg(self): | ||
| from diffusers import AutoPipelineForText2Image | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 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 wdyt?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DN6
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But how is this count useful to a contributor / maintainer, though? Could you provide an example? |
||
|
|
||
| pipeline = AutoPipelineForText2Image.from_pretrained(self.repo_id, enable_pag=True, torch_dtype=torch.float16) | ||
| pipeline.enable_model_cpu_offload(device=torch_device) | ||
| pipeline.set_progress_bar_config(disable=None) | ||
|
|
@@ -271,6 +272,8 @@ def test_pag_cfg(self): | |
| ) | ||
|
|
||
| def test_pag_uncond(self): | ||
| from diffusers import AutoPipelineForText2Image | ||
|
|
||
| pipeline = AutoPipelineForText2Image.from_pretrained(self.repo_id, enable_pag=True, torch_dtype=torch.float16) | ||
| pipeline.enable_model_cpu_offload(device=torch_device) | ||
| pipeline.set_progress_bar_config(disable=None) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fetcher is currently in
workflow_dispatch. Should it have apull_requesttrigger? And then we should also make sure that the fetching script is always run from the base upstream (for security reasons)?