Conversation
Summary of ChangesHello @gushiqiao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a major refactor of the Gradio user interface and underlying model loading logic. The primary goal is to enhance usability by shifting model and task configuration from command-line arguments to interactive UI elements. This includes dynamic model discovery, automatic quantization detection, and intelligent auto-configuration of inference parameters based on hardware capabilities and selected model characteristics. The changes streamline the user experience, making it easier to experiment with different models and settings without manual script modifications, while also improving memory management and providing clearer feedback on model loading requirements. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a major refactoring of the Gradio demo application, significantly improving its flexibility and user experience. The key changes include moving model and task selection from command-line arguments to a dynamic UI, which now discovers available models from the filesystem. The UI has been redesigned for better clarity, and the underlying configuration logic, including auto-configuration of performance settings, has been simplified and made more robust. The startup scripts and documentation have also been updated to reflect these changes.
My review focuses on improving code maintainability by reducing duplication in the new helper functions and enhancing the robustness of resource management in the offloading manager. Overall, this is a very positive set of changes that makes the Gradio demo much more powerful and easier to use.
| def __del__(self): | ||
| if hasattr(self, "executor") and self.executor is not None: | ||
| # Wait for all pending futures to complete before shutting down | ||
| if hasattr(self, "prefetch_futures"): | ||
| for f in self.prefetch_futures: | ||
| try: | ||
| if not f.done(): | ||
| f.result() | ||
| except Exception: | ||
| pass | ||
| self.executor.shutdown(wait=wait) | ||
| for f in self.prefetch_futures: | ||
| if not f.done(): | ||
| f.result() | ||
| self.executor.shutdown(wait=False) | ||
| self.executor = None | ||
| logger.debug("ThreadPoolExecutor shut down successfully.") |
There was a problem hiding this comment.
The shutdown logic has been moved into __del__. Relying on __del__ for resource cleanup is fragile, as its execution is not guaranteed in all circumstances. Additionally, the try...except block around f.result() was removed. If a future has an exception, it will be raised inside __del__, which can lead to unhandled exceptions and warnings from the interpreter. It's safer to catch and log potential exceptions here.
For more robust resource management, consider using a context manager (__enter__ and __exit__) instead of __del__.
| def __del__(self): | |
| if hasattr(self, "executor") and self.executor is not None: | |
| # Wait for all pending futures to complete before shutting down | |
| if hasattr(self, "prefetch_futures"): | |
| for f in self.prefetch_futures: | |
| try: | |
| if not f.done(): | |
| f.result() | |
| except Exception: | |
| pass | |
| self.executor.shutdown(wait=wait) | |
| for f in self.prefetch_futures: | |
| if not f.done(): | |
| f.result() | |
| self.executor.shutdown(wait=False) | |
| self.executor = None | |
| logger.debug("ThreadPoolExecutor shut down successfully.") | |
| def __del__(self): | |
| if hasattr(self, "executor") and self.executor is not None: | |
| for f in self.prefetch_futures: | |
| try: | |
| if not f.done(): | |
| f.result() | |
| except Exception as e: | |
| logger.warning(f"Exception in prefetch future during shutdown: {e}") | |
| self.executor.shutdown(wait=False) | |
| self.executor = None | |
| logger.debug("ThreadPoolExecutor shut down successfully.") |
| if model_type == "wan2.1": | ||
| # wan2.1: filter files/dirs containing wan2.1 or Wan2.1 | ||
| def is_valid(name): | ||
| name_lower = name.lower() | ||
| if "wan2.1" not in name_lower: | ||
| return False | ||
| if not fp8_supported and "fp8" in name_lower: | ||
| return False | ||
| return not any(kw in name_lower for kw in excluded_keywords) | ||
| else: | ||
| paths_to_check.append(os.path.join(model_path, subdir, filename)) | ||
| print(paths_to_check) | ||
| for path in paths_to_check: | ||
| if os.path.exists(path): | ||
| logger.info(f"Found PyTorch model checkpoint: {path}") | ||
| return path | ||
| raise FileNotFoundError(f"PyTorch model file '{filename}' not found.\nPlease download the model from https://huggingface.co/lightx2v/ or specify the model path in the configuration file.") | ||
| # wan2.2: filter files/dirs containing wan2.2 or Wan2.2 | ||
| def is_valid(name): | ||
| name_lower = name.lower() | ||
| if "wan2.2" not in name_lower: | ||
| return False | ||
| if not fp8_supported and "fp8" in name_lower: | ||
| return False | ||
| return not any(kw in name_lower for kw in excluded_keywords) |
There was a problem hiding this comment.
There is significant code duplication in this function. The is_valid nested function is defined twice with almost identical logic for model_type == 'wan2.1' and the else block. This pattern of duplication is also present across other get_*_choices functions (e.g., get_high_noise_choices, get_t5_choices).
To improve maintainability, this can be refactored. For get_dit_choices, you can define is_valid once and use the model_type variable from the outer scope. For the other functions, consider creating a single, more generic helper function that takes keywords as an argument to handle the filtering logic.
def is_valid(name):
name_lower = name.lower()
if model_type not in name_lower:
return False
if not fp8_supported and "fp8" in name_lower:
return False
return not any(kw in name_lower for kw in excluded_keywords)| if model_type == "wan2.1": | ||
| # wan2.1: 筛选包含 wan2.1 或 Wan2.1 的文件/目录 | ||
| def is_valid(name): | ||
| name_lower = name.lower() | ||
| if "wan2.1" not in name_lower: | ||
| return False | ||
| if not fp8_supported and "fp8" in name_lower: | ||
| return False | ||
| return not any(kw in name_lower for kw in excluded_keywords) | ||
| else: | ||
| paths_to_check.append(os.path.join(model_path, subdir, filename)) | ||
| print(paths_to_check) | ||
| for path in paths_to_check: | ||
| if os.path.exists(path): | ||
| logger.info(f"Found PyTorch model checkpoint: {path}") | ||
| return path | ||
| raise FileNotFoundError(f"PyTorch model file '{filename}' not found.\nPlease download the model from https://huggingface.co/lightx2v/ or specify the model path in the configuration file.") | ||
| # wan2.2: 筛选包含 wan2.2 或 Wan2.2 的文件/目录 | ||
| def is_valid(name): | ||
| name_lower = name.lower() | ||
| if "wan2.2" not in name_lower: | ||
| return False | ||
| if not fp8_supported and "fp8" in name_lower: | ||
| return False | ||
| return not any(kw in name_lower for kw in excluded_keywords) |
There was a problem hiding this comment.
此函数中存在大量重复代码。is_valid 嵌套函数在 model_type == 'wan2.1' 和 else 分支中被定义了两次，其逻辑几乎完全相同。这种重复模式也存在于其他的 get_*_choices 函数中（例如 get_high_noise_choices, get_t5_choices）。
为了提高可维护性，可以对此进行重构。对于 get_dit_choices，您可以只定义一次 is_valid 函数，并使用外部作用域的 model_type 变量。对于其他函数，可以考虑创建一个更通用的辅助函数，接收关键字作为参数来处理过滤逻辑。
def is_valid(name):
name_lower = name.lower()
if model_type not in name_lower:
return False
if not fp8_supported and "fp8" in name_lower:
return False
return not any(kw in name_lower for kw in excluded_keywords)2 participants
No description provided.