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 check_requirements() resource warning allocation open file #5602

Merged
merged 2 commits into from
Nov 10, 2021

Conversation

ayman-saleh
Copy link
Contributor

@ayman-saleh ayman-saleh commented Nov 10, 2021

The file.open usage was throwing ResourceWarning Exceptions within our internal testing framework, tracked down to the unclosed file handler while checking the requirements.txt file.

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Improvement in handling requirements file parsing in utils/general.py.

📊 Key Changes

  • Shift from a direct file open approach to utilizing a context manager for reading requirements.txt.

🎯 Purpose & Impact

  • Purpose: Ensure that the requirements file is properly closed after its contents are read, which adheres to best coding practices and avoids potential resource leaks.
  • Impact: This update results in cleaner code and more reliable resource management, but it should not affect the functionality from the user's perspective. Maintainers and developers, however, will benefit from code that is more robust and maintainable. 👍🔒

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Hello @ayman-saleh, thank you for submitting a 🚀 PR! To allow your work to be integrated as seamlessly as possible, we advise you to:

  • ✅ Verify your PR is up-to-date with upstream/master. If your PR is behind upstream/master an automatic GitHub actions rebase may be attempted by including the /rebase command in a comment body, or by running the following code, replacing 'feature' with the name of your local branch:
git remote add upstream https://github.com/ultralytics/yolov5.git
git fetch upstream
git checkout feature  # <----- replace 'feature' with local branch name
git merge upstream/master
git push -u origin -f
  • ✅ Verify all Continuous Integration (CI) checks are passing.
  • ✅ Reduce changes to the absolute minimum required for your bug fix or feature addition. "It is not daily increase but daily decrease, hack away the unessential. The closer to the source, the less wastage there is." -Bruce Lee

in keeping with naming convention
@glenn-jocher
Copy link
Member

@ayman-saleh good catch! Thanks for the PR.

@glenn-jocher glenn-jocher changed the title Fix to resource warning allocation open file within a context manager. Fix check_requirements() resource warning allocation open file Nov 10, 2021
@glenn-jocher glenn-jocher merged commit 27bf428 into ultralytics:master Nov 10, 2021
BjarneKuehl pushed a commit to fhkiel-mlaip/yolov5 that referenced this pull request Aug 26, 2022
…ralytics#5602)

* Fix to resource warning allocation; utilize file.open within a context manager

* rename fh to f

in keeping with naming convention

Co-authored-by: Ayman Saleh <aymansaleh@Aymans-MacBook-Pro-2.local>
Co-authored-by: Glenn Jocher <glenn.jocher@ultralytics.com>
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