-
Notifications
You must be signed in to change notification settings - Fork 229
[Feature] Ignore [union-attr] and [override] mypy check and remove from training #652
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
Conversation
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.
Summary of Changes
Hello @BrianChen1129, 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 primarily focuses on adjusting the mypy static analysis configuration to ignore specific error codes (union-attr and override). This change is accompanied by a significant cleanup of assert statements across various training pipeline files, which were likely either redundant for runtime logic or were causing issues with mypy's type inference. The overall effect is to reduce mypy noise and potentially simplify the codebase by removing assertions that are no longer strictly necessary for type checking under the new configuration.
Highlights
- Mypy Configuration Update: I've updated the
.pre-commit-config.yamlto configure themypyhook to disable checks forunion-attrandoverrideerror codes. This change aims to reduce noise from these specific type checking issues. - Assertion Statement Refinement: Across multiple training pipeline files (
distillation_pipeline.py,training_pipeline.py,wan_i2v_distillation_pipeline.py,wan_i2v_training_pipeline.py), I've removed numerousassertstatements that checked forNonevalues. This cleanup streamlines the code, as these assertions were either redundant for runtime logic or were causingmypytype inference challenges. In some cases, assertions were strategically moved or re-added to ensure critical type guarantees are still explicitly stated formypy's flow analysis. - Type Hint Adjustments: I've made minor adjustments to type hints, such as changing
dict[str, torch.Tensor] | Nonetodict[str, Any] | Noneindistillation_pipeline.pyand todict[str, torch.Tensor]inwan_i2v_distillation_pipeline.py. These changes reflect more accurate or flexible type expectations in specific contexts, likely in conjunction with themypyconfiguration updates.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request aims to silence mypy errors for union-attr and override by disabling them globally in the pre-commit configuration and removing the assert statements that were triggering them. My review focuses on why this approach is detrimental to code quality and safety. I've provided critical feedback to revert the global mypy suppression and instead fix the underlying type issues in the code, which includes restoring necessary runtime checks and correcting incompatible method signatures.
| hooks: | ||
| - id: mypy | ||
| args: [--python-version, '3.10', --follow-imports, "skip" ] | ||
| args: [--python-version, '3.10', --follow-imports, "skip", "--disable-error-code", "union-attr", "--disable-error-code", "override" ] |
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.
Disabling mypy error codes globally is a dangerous practice that can mask serious bugs and decrease the overall quality and maintainability of the codebase. Instead of suppressing these checks, the underlying issues should be addressed directly in the code.
union-attr: This error prevents potentialAttributeError: 'NoneType' object has no attribute '...'exceptions at runtime. The numerousassert var is not Nonestatements that were removed in this PR were likely serving as type guards to prevent this error. The correct solution is to keep these assertions or useifchecks to narrow the type.override: This error indicates a method signature in a subclass is incompatible with its superclass, which violates the Liskov Substitution Principle. This can lead to unexpected behavior when using polymorphism. The fix is to make the signatures compatible.
Please revert this change and address the mypy errors in the Python code.
args: [--python-version, '3.10', --follow-imports, "skip" ]
| self.vae = self.get_module("vae") | ||
| self.vae.requires_grad_(False) | ||
|
|
||
| assert self.training_args is not 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.
This assertion, and others like it that have been removed throughout the PR, served an important purpose. It acted as a runtime check to prevent NoneType errors and also as a type guard for mypy's static analysis.
By removing this check and globally disabling the [union-attr] mypy error, we are hiding a potential runtime crash and making the code harder to reason about.
If self.training_args is guaranteed to be non-None at this point in the code, the type hint for the attribute on the class should be updated from Optional[TrainingArgs] to TrainingArgs. If it can be None, this check (or a more robust if/raise) should be restored to ensure runtime safety. This principle applies to most of the assertions removed in this pull request.
| def _build_distill_input_kwargs( | ||
| self, noise_input: torch.Tensor, timestep: torch.Tensor, | ||
| text_dict: dict[str, torch.Tensor] | None, | ||
| text_dict: dict[str, torch.Tensor], |
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.
This signature is incompatible with the parent class DistillationPipeline, where text_dict is optional (... | None). This violates the Liskov Substitution Principle and is likely the cause of the [override] mypy error that is being suppressed. A subclass method's parameters should not be more restrictive than the superclass's.
Please revert the signature to be compatible with the parent and handle the None case within the method body, for instance by re-adding an assertion or a ValueError check.
| text_dict: dict[str, torch.Tensor], | |
| text_dict: dict[str, torch.Tensor] | None, |
No description provided.