Skip to content
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

Fix pylint: do not use bare 'except' #5025

Merged
merged 18 commits into from
Oct 4, 2021

Conversation

zhiqwang
Copy link
Contributor

@zhiqwang zhiqwang commented Oct 1, 2021

Hi @glenn-jocher , I'm fixing the lint in this PR, as flake8 will raise

E722 do not use bare 'except'

πŸ› οΈ PR Summary

Made with ❀️ by Ultralytics Actions

🌟 Summary

Refinement of exception handling and cleanup in YOLOv5 codebase.

πŸ“Š Key Changes

  • Specific exception handling using except NameError instead of a general exception clause.
  • Removal of an unnecessary import statement from train.py.
  • Enhancement in the print_mutation function for better readability in hyperparameter evolution results documentation.

🎯 Purpose & Impact

  • πŸ› οΈ Better Error Handling: Catching specific exceptions improves code reliability and maintainability, making it easier to debug issues.
  • 🧹 Code Cleanup: Removing unused imports declutters the code, which may lead to slight performance improvements and better understanding for developers.
  • ✏️ Improved Documentation: Formatting changes in print_mutation function result in cleaner and more professional representation of hyperparameter evolution results.
  • πŸ“¦ Overall: These changes are likely to enhance the YOLOv5 experience for developers by making it more robust and user-friendly. Non-expert users benefit indirectly through a more stable and polished tool.

@glenn-jocher
Copy link
Member

@zhiqwang thanks for the PR! Is this ready to review?

@zhiqwang
Copy link
Contributor Author

zhiqwang commented Oct 2, 2021

Is this ready to review?

Hi @glenn-jocher , Yep, is ready!

@glenn-jocher
Copy link
Member

glenn-jocher commented Oct 3, 2021

@zhiqwang ok I think some of these are good, but some may cause issues. Some of the Try except statements are really meant to handle a variety of possible errors.

For example the cache try-except on datasets L412, multiple things could go wrong that are all currently handled by the bare try-except statement. 1) The file might not be found, the 2) file might just be corrupt and not load correctly, the asserts might fail if 3) the cache version is incorrect or 4) if the cache hash is different than the dataset hash. These are the known failure modes, but there may be other unknown failure modes that I'm not aware of right now. All of these failure modes (known and unknown) are intended to be caught by the existing bare try-except statement and trigger a new re-caching.

So while it might be nice to more explicitly create a tuple of errors above, it also makes the code more brittle and less robust in practice since we are only including what we know might go wrong.

thop is another similar case. sometimes thop fails on certain models/environments, so the try except is not just in place to catch import errors but also any possible thop error, which is out of our control.

Let's do this. Can you remove all try-except updates except for the very simple cases where there is only 1 possible error to be caught: tf.py, yolo.py, general.py. Also please revert plots.py L311, as the y output is meant to be used on L312 to color the class histogram once the matplotlib issue is resolved. Thank you!

@zhiqwang
Copy link
Contributor Author

zhiqwang commented Oct 3, 2021

Hi @glenn-jocher

Thanks for the detailed explanation, and I have applied the changes you suggested.

@glenn-jocher glenn-jocher merged commit 1922dde into ultralytics:master Oct 4, 2021
@glenn-jocher
Copy link
Member

@zhiqwang changes look good!

PR is merged. Thank you for your contributions to YOLOv5 πŸš€ and Vision AI ⭐

@zhiqwang zhiqwang deleted the handle-exception branch October 4, 2021 01:23
BjarneKuehl pushed a commit to fhkiel-mlaip/yolov5 that referenced this pull request Aug 26, 2022
* Fix E722, do not use bare 'except'

* Remove used codes

* Add FileNotFoundError in LoadImagesAndLabels

* Remove AssertionError

* Ignore LoadImagesAndLabels

* Ignore downloads.py

* Ignore torch_utils.py

* Ignore train.py

* Ignore datasets.py

* Enable utils/download.py

* Fixing exception in thop

* Remove unused code

* Fixing exception in LoadImagesAndLabels

* Fixing exception in exif_size

* Fixing exception in parse_model

* Ignore exceptions in requests

* Revert the exception as suggested

* Revert the exception as suggested
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants