Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors Qwen image pipeline tests to use the unified DiffSynthEngine and QwenImagePipelineConfig, and adds new test cases for parallelism and FSDP. Feedback recommends explicitly calling engine.shutdown() in teardown methods for resource cleanup and requests clarification on the reduction of layers and threshold adjustments in the layered image test.
| @classmethod | ||
| def tearDownClass(cls): | ||
| del cls.pipe | ||
| del cls.engine |
There was a problem hiding this comment.
Explicitly calling cls.engine.shutdown() before deleting the engine is recommended to ensure that worker processes are terminated immediately and resources (like GPU memory and multiprocessing pipes) are released. Relying solely on del and garbage collection can be unreliable in some environments, potentially leading to resource leaks or port conflicts in subsequent tests.
| del cls.engine | |
| cls.engine.shutdown() | |
| del cls.engine |
|
|
||
| @classmethod | ||
| def tearDownClass(cls): | ||
| del cls.engine |
There was a problem hiding this comment.
Explicitly calling cls.engine.shutdown() before deleting the engine is recommended to ensure that worker processes are terminated immediately and resources are released.
| del cls.engine | |
| cls.engine.shutdown() | |
| del cls.engine |
|
|
||
| @classmethod | ||
| def tearDownClass(cls): | ||
| del cls.engine |
There was a problem hiding this comment.
Explicitly calling cls.engine.shutdown() before deleting the engine is recommended to ensure that worker processes are terminated immediately and resources are released.
| del cls.engine | |
| cls.engine.shutdown() | |
| del cls.engine |
| @classmethod | ||
| def tearDownClass(cls): | ||
| del cls.pipe | ||
| del cls.engine |
There was a problem hiding this comment.
Explicitly calling cls.engine.shutdown() before deleting the engine is recommended to ensure that worker processes are terminated immediately and resources are released.
| del cls.engine | |
| cls.engine.shutdown() | |
| del cls.engine |
|
|
||
| @classmethod | ||
| def tearDownClass(cls): | ||
| del cls.engine |
There was a problem hiding this comment.
Explicitly calling cls.engine.shutdown() before deleting the engine is recommended to ensure that worker processes are terminated immediately and resources are released.
| del cls.engine | |
| cls.engine.shutdown() | |
| del cls.engine |
| @classmethod | ||
| def tearDownClass(cls): | ||
| del cls.pipe | ||
| del cls.engine |
There was a problem hiding this comment.
Explicitly calling cls.engine.shutdown() before deleting the engine is recommended to ensure that worker processes are terminated immediately and resources are released.
| del cls.engine | |
| cls.engine.shutdown() | |
| del cls.engine |
| @classmethod | ||
| def tearDownClass(cls): | ||
| del cls.pipe | ||
| del cls.engine |
There was a problem hiding this comment.
Explicitly calling cls.engine.shutdown() before deleting the engine is recommended to ensure that worker processes are terminated immediately and resources are released.
| del cls.engine | |
| cls.engine.shutdown() | |
| del cls.engine |
| @classmethod | ||
| def tearDownClass(cls): | ||
| del cls.pipe | ||
| del cls.engine |
There was a problem hiding this comment.
Explicitly calling cls.engine.shutdown() before deleting the engine is recommended to ensure that worker processes are terminated immediately and resources are released.
| del cls.engine | |
| cls.engine.shutdown() | |
| del cls.engine |
| num_inference_steps=50, | ||
| true_cfg_scale=4.0, | ||
| layers=4, | ||
| layers=3, |
There was a problem hiding this comment.
The number of layers has been reduced from 4 to 3, which is also reflected in the assertion on line 40. Since 4 is the default value for the layers parameter in QwenImageLayeredPipeline, could you clarify why the test was changed to use 3 layers? If this was done to align with changed model behavior on Torch 2.10.0, it might be masking a regression in the model's ability to produce the expected number of layers.
1 participant
This pull request refactors the test suites for the Qwen image pipelines to use the unified
DiffSynthEngineinterface and configuration objects, and adds parallelism and FSDP (Fully Sharded Data Parallel) test coverage. The changes improve consistency, extensibility, and maintainability of the tests, while also ensuring GPU memory is properly released after each test class.Test pipeline refactoring and improvements:
All Qwen image pipeline tests now use
DiffSynthEnginewithQwenImagePipelineConfiginstead of individual pipeline classes, improving consistency and future extensibility. (tests/test_pipelines/test_qwen_image.py,tests/test_pipelines/test_qwen_image_edit.py,tests/test_pipelines/test_qwen_image_edit_plus_2509.py,tests/test_pipelines/test_qwen_image_edit_plus_2511.py,tests/test_pipelines/test_qwen_image_layered.py) [1] [2] [3] [4] [5]The
.generate()method ofDiffSynthEnginereplaces direct pipeline calls, standardizing how image generation and editing are invoked in all tests. [1] [2] [3] [4] [5]Parallelism and FSDP test coverage:
Test resource management:
tearDownClass, reducing the risk of out-of-memory errors in CI or local runs. [1] [2] [3] [4] [5]Layered pipeline test adjustments:
Overall, these changes modernize and strengthen the test suite for Qwen image pipelines, aligning it with the latest engine interface and distributed features.