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

thop and pycocotools shouldn't be hard requirements to train a model #2014

Closed
Ownmarc opened this issue Jan 22, 2021 · 6 comments
Closed

thop and pycocotools shouldn't be hard requirements to train a model #2014

Ownmarc opened this issue Jan 22, 2021 · 6 comments
Labels
bug Something isn't working Stale

Comments

@Ownmarc
Copy link
Contributor

Ownmarc commented Jan 22, 2021

Not having thop and pycocotools installed shouldn't prevent me from starting a training.

pycocotools is never required by train.py
thop is optional

image

@Ownmarc Ownmarc added the bug Something isn't working label Jan 22, 2021
@glenn-jocher
Copy link
Member

glenn-jocher commented Jan 22, 2021

@Ownmarc hey there. You raise a good point. We added pycocotools because often users would only return the repo mAP on COCO and be confused when the repo mAP did not match the README mAP, which is obtained from pycocotools, but you are right the requirement is not needed for all other datasets.

thop is used to compute GFLOPS for model summaries, but is not strictly required for training either. In general we've tried to balance the requirements with the feature additions they provide.

The requirements check we also instituted after too many users were reporting 'bugs' due to not meeting dependency version, i.e. training with pytorch 1.4 and then raising a bug report (without mentioning package versions) where the error message is hard to trace back to a version mismatch, like 'nn.SiLU() module not found' etc.

@glenn-jocher
Copy link
Member

On an unrelated note I see a github check problem in your screenshot as well... hmm I'll take a look at the CI windows results there. Yes, it appears in windows CI also. https://github.com/ultralytics/yolov5/runs/1745848780?check_suite_focus=true

Ok I'll add a TODO for this: windows github check fix.

@glenn-jocher
Copy link
Member

glenn-jocher commented Jan 22, 2021

The problem seems related to inlining the current branch name with the $ symbol:
cmd = 'git rev-list $(git rev-parse --abbrev-ref HEAD)..origin/master --count' # commits behind

problem appears to be $(git rev-parse --abbrev-ref HEAD)

Possible solution in https://stackoverflow.com/questions/6245570/how-to-get-the-current-branch-name-in-git
https://superuser.com/questions/289344/is-there-something-like-command-substitution-in-windows-cli

@Ownmarc
Copy link
Contributor Author

Ownmarc commented Jan 22, 2021

so pycocotools should only be a requirement when test.py is calling check_requirements() and that the opt.save_json is True

Could probably do that :

def check_requirements(file='requirements.txt', require_pycoco=False):
    import pkg_resources
    _requirements = pkg_resources.parse_requirements(Path(file).open())
    requirements = []
    for requirement in requirements:
        if 'pycocotools' in requirement.name and not require_pycoco:
            continue
        elif 'thop' == requirement.name.lower():
            continue
        else:
            if len(x.specs):
                requirements.append(requirement.name + ''.join(*requirement.specs))
            else:
                requirements.append(requirement.name)
    
    print(requirements)
    pkg_resources.require(requirements)

I'm sure you'll come up with prettier code ahah

and in test.py :
check_requirements(require_pycoco=opt.save_json)

@glenn-jocher
Copy link
Member

glenn-jocher commented Jan 22, 2021

@Ownmarc thanks for the feedback, we will take this into consideration.

In the meantime I've pushed a fix for the github check error, this should work well in Windows now. This check advises you of updates to master, which we've implemented to help increase awareness of code improvements and to encourage more people to update.

@glenn-jocher glenn-jocher removed the TODO label Jan 24, 2021
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Stale
Projects
None yet
Development

No branches or pull requests

2 participants