fix: raise NotImplementedError and avoid mutable default args#6363
fix: raise NotImplementedError and avoid mutable default args#6363Mr-Neutr0n wants to merge 3 commits intoespnet:masterfrom
Conversation
…table default arguments
In several abstract base classes, `return NotImplementedError` was used
instead of `raise NotImplementedError`, which silently returns the
exception class as a value rather than raising it. This means subclasses
that forget to override the `name` property would not get an error.
Also replace mutable default argument `others={}` with `others=None`
in enhancement loss wrapper forward() methods to avoid the well-known
Python mutable default argument pitfall.
Affected files:
- espnet2/s2st/losses/abs_loss.py
- espnet2/s2st/aux_attention/abs_aux_attention.py
- espnet2/enh/loss/criterions/abs_loss.py
- espnet2/asvspoof/loss/abs_loss.py
- espnet2/uasr/loss/abs_loss.py
- espnet2/enh/loss/wrappers/pit_solver.py
- espnet2/enh/loss/wrappers/dpcl_solver.py
- espnet2/enh/loss/wrappers/multilayer_pit_solver.py
- espnet2/enh/loss/wrappers/fixed_order.py
There was a problem hiding this comment.
Code Review
This pull request correctly addresses two important issues: replacing return NotImplementedError with raise NotImplementedError in abstract base classes to ensure proper error handling, and fixing the use of mutable default arguments, which is a common source of bugs in Python.
The changes are well-implemented. However, the fix for mutable default arguments in the wrapper classes highlights a pre-existing issue with method signature mismatches between these classes and their AbsLossWrapper base class. The forward method in the subclasses has a signature that is incompatible with the one defined in the abstract base class, violating the Liskov Substitution Principle. I've added comments with high severity to these files, recommending that the base class be updated to ensure type consistency and maintainability across the codebase.
| self.weight = weight | ||
|
|
||
| def forward(self, ref, inf, others={}): | ||
| def forward(self, ref, inf, others=None): |
There was a problem hiding this comment.
While changing the mutable default argument is correct, this method's signature is incompatible with the base class AbsLossWrapper. The base class defines forward(self, ..., others: Dict), making others a required argument. This implementation makes it optional (others=None). This violates the Liskov Substitution Principle and can cause issues with static analysis tools. To fix this properly, the base class AbsLossWrapper should be updated to def forward(self, ..., others: Optional[Dict] = None) to reflect that others is optional. This should be addressed to ensure type consistency.
| self.weight = weight | ||
|
|
||
| def forward(self, ref, inf, others={}): | ||
| def forward(self, ref, inf, others=None): |
There was a problem hiding this comment.
This change to avoid a mutable default argument is good, but it reveals a signature mismatch with the base class AbsLossWrapper. The base class expects others as a required Dict argument, but here it is optional (others=None). This violates the Liskov Substitution Principle. For type safety, the signature in the subclass should be compatible with the superclass. Consider updating the base class AbsLossWrapper to define others as optional: others: Optional[Dict] = None.
| self.layer_weights = layer_weights | ||
|
|
||
| def forward(self, ref, infs, others={}): | ||
| def forward(self, ref, infs, others=None): |
There was a problem hiding this comment.
While fixing the mutable default argument is correct, this method signature has a more fundamental incompatibility with the base class AbsLossWrapper. The base class signature is forward(self, ref: List, inf: List, others: Dict). This implementation's signature is forward(self, ref, infs, others=None).
There are two inconsistencies:
- The second argument is
infsinstead ofinf, with a different type. othersis made optional, while it's required in the base class.
This violates the Liskov Substitution Principle. The inheritance relationship should be reviewed. If MultiLayerPITSolver should conform to the AbsLossWrapper interface, its forward method signature must be compatible. This might require refactoring the base class or the inheritance structure.
| self.flexible_numspk = flexible_numspk | ||
|
|
||
| def forward(self, ref, inf, others={}): | ||
| def forward(self, ref, inf, others=None): |
There was a problem hiding this comment.
Fixing the mutable default argument is a good improvement. However, this change makes the method signature incompatible with the AbsLossWrapper base class. The base class specifies others: Dict as a required argument, while this implementation makes it optional. This violates the Liskov Substitution Principle. The base class AbsLossWrapper should ideally be changed to ... others: Optional[Dict] = None to make the contract clear and consistent across subclasses.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6363 +/- ##
==========================================
+ Coverage 69.53% 69.63% +0.10%
==========================================
Files 774 775 +1
Lines 70978 71545 +567
==========================================
+ Hits 49352 49820 +468
- Misses 21626 21725 +99
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
Fix
return NotImplementedError->raise NotImplementedErrorin 5 abstract base classes. Usingreturnsilently returns the exception class object as a value instead of raising it, so subclasses that forget to override thenameproperty would not get an error at runtime.Affected files:
espnet2/s2st/losses/abs_loss.pyespnet2/s2st/aux_attention/abs_aux_attention.pyespnet2/enh/loss/criterions/abs_loss.pyespnet2/asvspoof/loss/abs_loss.pyespnet2/uasr/loss/abs_loss.pyFix mutable default argument
others={}->others=Nonewith aNoneguard in 4 enhancement loss wrapperforward()methods. Mutable default arguments in Python are shared across all calls, which can lead to subtle bugs if the dict is ever mutated.Affected files:
espnet2/enh/loss/wrappers/pit_solver.pyespnet2/enh/loss/wrappers/dpcl_solver.pyespnet2/enh/loss/wrappers/multilayer_pit_solver.pyespnet2/enh/loss/wrappers/fixed_order.pyTest plan
pytest test/espnet2/enh/loss/)