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

Add yolov5 namespace #2886

Closed
wants to merge 3 commits into from

Conversation

sheromon
Copy link

@sheromon sheromon commented Apr 22, 2021

The changes here to address #2481 move the yolov5 code into a yolov5 directory with an __init__.py file to make yolov5 a Python package. When yolov5 is installed using pip, modules and functions can then be imported like from yolov5.utils.autoanchor import check_anchors with no need to add any directories to the Python path. This is a problem that many users have encountered in the past as in #134 and #353.

Adding yolov5 to the import statement can also help to disambiguate in the event that a user has something else named models or utils in their Python path.

These changes are intentionally made to be consistent with changes made by @fcakyon in yolov5-pip and make the code in these two repos more similar.

A TODO item is updating the docs to reflect the new locations for various files. I thought about doing that but figured it made more sense to get some feedback on these changes first.

πŸ› οΈ PR Summary

Made with ❀️ by Ultralytics Actions

🌟 Summary

Reorganization of the YOLOv5 codebase with directory structure changes and updates to files for better modularity.

πŸ“Š Key Changes

  • πŸ“ Refactored the project structure by moving most files into a new yolov5 directory.
  • 🧹 Cleaned up import statements to reflect the new directory structure.
  • βœ‚οΈ Removed unnecessary sys.path.append calls due to the reorganization.
  • βž• Added .vscode to .gitignore to prevent committing Visual Studio Code-specific files.
  • πŸ”„ Adjusted hard-coded paths in the .gitignore to match the new structure.

🎯 Purpose & Impact

  • βš™οΈ Enhances modularity: better organization makes it easier to maintain and navigate the project.
  • πŸ” Improves clarity: clear separation of different components for developers working on the project.
  • πŸš€ Project scalability: sets a foundation for future expansions/extensions of the codebase.
  • ✨ User experience: non-intrusive changes from a user perspective, but may require updates to custom import statements if working directly with the code.

…statements as in yolov5-pip repo.

Note: I had to make a few changes from what is in yolov5-pip because there appear to have been some code changes between the version in yolov5-pip and where I branched off of the yolov5 repo. While I was updating import statements, I noticed that there are still a couple of places in .py files where the import path isn't updated to include the yolov5 namespace in yolov5-pip, and I stayed consistent with what's in yolov5-pip by not updating those import statements.
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 @sheromon, 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 origin/master. If your PR is behind origin/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 rebase 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

@glenn-jocher
Copy link
Member

glenn-jocher commented Apr 23, 2021

@sheromon thanks for the PR! Users have made similar suggestions in the past, including @fcakyon and @Borda with an earlier PR. We'd like to strike a balance between exportability/deployability and training/development so we've avoided this so far, thinking it would complicate matters for many use-cases.

Is there a middle-ground that might work so that someone could clone yolov5 within their own project and then use the same import structure you have above? Perhaps if we made the imports relative rather than absolute?

@fcakyon
Copy link
Member

fcakyon commented Apr 23, 2021

@glenn-jocher im not 100% sure but relative imports might make it easier

@glenn-jocher
Copy link
Member

glenn-jocher commented Apr 23, 2021

@fcakyon @sheromon, I'm not really sure either. I understand of course that dropping everything into a subfolder would make packaging easier, but it would also create more hurdles and possible failure points when new users were trying to learn and reproduce the tutorials, i.e. the current command is hard to fail, even if no arguments are passed we've designed it to run correctly the first time, and there's no ambiguity really about where to run it from:

$ python train.py

But under the proposed PR there would be 4 possible cases and probably many more bug reports from confused users in the future (especially first-time uses, which we really focus on):

$ python train.py  # works from yolov5/ but fails from .
$ python yolov5/train.py  # works from . but fails from yolov5/

EDIT: if the git clone directory and package directory are identically named it might lead to further confusion also, i.e. user raises an issue saying "I ran this command from the yolov5/ directory and {bug here}" and we would not know what that meant.

@Borda
Copy link
Contributor

Borda commented Apr 23, 2021

Check make installable pip package #465

@sheromon
Copy link
Author

sheromon commented Apr 28, 2021

@glenn-jocher, oh, I hadn't realized that this change had been proposed before.

Now that you mention it, I can see the concern about confusion about which directory to run from. It seemed pretty clear to me, but I guess I'm somewhat used to working with Python code that is set up to be a pip package now. It's true that I don't have experience maintaining code for a large number of users -- I mainly write code for myself and my teammates, so I recognize that you have the much more experienced perspective.

I feel like relative imports wouldn't work, because sometimes you'd need to include a parent directory in the import path, e.g. in models/common.py, there's this line: from utils.datasets import letterbox. A relative import would have to go up a level from models, so I think it would be from ...utils.datasets import letterbox, and that seems like it wouldn't work if you don't have the pip package installed, right?

Anyway, the motivation was to solve the import issues that I and some others had encountered. To fix my problem for just me, I could probably just work off of the yolov5-pip repo instead.

Update: Oh! Maybe if this repo mentioned yolov5-pip? I just saw the part in #465 where someone said they'd found yolov5-pip but weren't sure "how much (if at all) it was associated with this project". It seems like it would be helpful to at least some people if maybe the ultralytics/yolov5 repo mentioned yolov5-pip so the relationship would be a little bit clearer.

@sheromon
Copy link
Author

@Borda, thanks for mentioning, but it looks like nothing's been going on with #465 since last November?

@SkalskiP SkalskiP changed the base branch from master to develop May 24, 2021 20:09
@sheromon sheromon closed this May 27, 2021
@sheromon sheromon deleted the add-yolov5-namespace branch May 27, 2021 21:28
@sheromon
Copy link
Author

I retract my request

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

4 participants