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: NotImplementedError: cannot instantiate 'PosixPath' on your system on Windows platform #12785

Open
wants to merge 59 commits into
base: master
Choose a base branch
from

Conversation

abdulhakkeempa
Copy link

@abdulhakkeempa abdulhakkeempa commented Mar 5, 2024

When the inference code was run on the Windows platform, it raised an error: NotImplementedError: cannot instantiate 'PosixPath' on your system. This issue has been fixed by adding three lines of code to detect.py. After testing, it was found to work fine. Closes #10240.

Changes made

# Updating PosixPath on Windows for compatibility
if sys.platform == "win32":
    temp = pathlib.PosixPath
    pathlib.PosixPath = pathlib.WindowsPath
    del temp

I have read the CLA Document and I hereby sign the CLA

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Improved Windows compatibility in YOLOv5's detect.py script 🔍💻

📊 Key Changes

  • Added a compatibility fix for Windows environments involving the pathlib module.

🎯 Purpose & Impact

  • The change ensures that the YOLOv5 detection script can run smoothly on Windows systems by addressing path handling issues.
  • Users on Windows will experience fewer issues related to file paths, leading to a more pleasant development or deployment experience.

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 @abdulhakkeempa, thank you for submitting a YOLOv5 🚀 PR! To allow your work to be integrated as seamlessly as possible, we advise you to:

  • ✅ Verify your PR is up-to-date with ultralytics/yolov5 master branch. If your PR is behind you can update your code by clicking the 'Update branch' button or by running git pull and git merge master locally.
  • ✅ Verify all YOLOv5 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

@glenn-jocher
Copy link
Member

@abdulhakkeempa thank you for your contribution and for highlighting the issue with Windows compatibility in the YOLOv5 detection script. Your effort to improve the project is greatly appreciated! 🌟

However, before we can proceed with integrating your changes, we need to review them thoroughly to ensure they align with the project's standards and do not introduce any unintended side effects. Compatibility fixes are crucial, and your approach seems promising. We'll look into the best way to incorporate this fix in a manner that maintains compatibility across all platforms.

In the meantime, could you please ensure that your changes are documented clearly, including any tests you've conducted to verify the fix? This will greatly aid in the review process.

Thanks again for your contribution and for supporting the YOLOv5 community! Your efforts help make YOLOv5 better for everyone. 🚀

@abdulhakkeempa
Copy link
Author

Hey @glenn-jocher, I have successfully run detect.py and it worked fine. I found this solution by reading the discussions in this repository related to this issue. Could you please suggest what type of test would be appropriate to check compatibility?

@glenn-jocher
Copy link
Member

Hello @abdulhakkeempa, it's great to hear that you've successfully run detect.py with your modifications and that it's working well. Your initiative to find a solution by engaging with the community discussions is commendable. 🙌

For testing compatibility, especially with changes that involve platform-specific behavior like file path handling, here are a few suggestions:

  1. Cross-Platform Testing: Ensure that the script runs successfully on multiple operating systems, not just Windows. Testing on Linux and macOS, if possible, would help verify that your changes do not negatively affect these environments.

  2. Automated Tests: If you're familiar with writing unit tests, consider adding a few that mock the sys.platform value to simulate different operating systems. This can help automatically verify that the correct Path class is used on each platform.

  3. Real-World Scenarios: Beyond basic functionality tests, try running the script in more complex scenarios that involve different types of file paths (e.g., paths with spaces, special characters, or network paths on Windows).

  4. Performance Impact: While not directly related to compatibility, it's good practice to check if the changes have any noticeable impact on the script's performance, especially since file path handling can be a frequent operation.

Documenting these tests and their outcomes will not only support the review process but also provide valuable information for future contributors who might encounter similar issues.

Thank you for your dedication to improving YOLOv5. Your efforts are truly appreciated by the community! 🌟

@glenn-jocher
Copy link
Member

@abdulhakkeempa we have Windows CI here that runs correctly on master branch, so we are unable to reproduce any Windows issues:
https://github.com/ultralytics/yolov5/actions/runs/8159399150/job/22303605439?pr=12785

@sohang3112
Copy link

@abdulhakkeempa we have Windows CI here that runs correctly on master branch, so we are unable to reproduce any Windows issues

@glenn-jocher AFAIK this issue with pathlib.Path occurs when the model was trained on a Unix platform, but inference is done on Windows (or vice versa). Does the Windows CI test for this?

By Unix platform, I mean Linux, Mac, Google Colab, etc.

@glenn-jocher
Copy link
Member

Hello @sohang3112,

Thanks for the clarification! 😊 Our current Windows CI does not specifically test for cross-platform model training and inference scenarios like Unix-to-Windows or vice versa. It is a great point and something we'll consider improving in our testing suite to ensure compatibility across all platforms.

In the meantime, your workaround with pathlib.Path adjustments looks promising for those encountering similar cross-platform path issues. For others reading, here's a condensed version of the proposed fix that could be applied before running inference:

import sys, pathlib
if sys.platform == "win32":
    pathlib.PosixPath = pathlib.WindowsPath

Keep those insights and contributions coming, they help make YOLOv5 better for everyone! 🚀

Copy link
Contributor

github-actions bot commented Apr 9, 2024


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I sign the CLA


2 out of 3 committers have signed the CLA.
✅ (glenn-jocher)[https://github.com/glenn-jocher]
✅ (UltralyticsAssistant)[https://github.com/UltralyticsAssistant]
@abdulhakkeempa
You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@sohang3112
Copy link

@abdulhakkeempa Please sign the CLA (Contributor License Agreement)

UltralyticsAssistant and others added 30 commits June 20, 2024 18:51
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.

NotImplementedError: cannot instantiate 'PosixPath' on your system
4 participants