search

cheind / py-motmetrics

:bar_chart: Benchmark multiple object trackers (MOT) in Python
MIT License
1.37k stars 259 forks source link

Distribution should not depend on Flake8 #98

Closed IRDonch closed 4 years ago

IRDonch commented 4 years ago

Currently, the list of dependencies of the motmetrics distribution is taken from the requirements.txt file, which includes flake8 and flake8-import-order. However, these packages are not used anywhere in the motmetrics code. This means that installing motmetrics leads to a bunch of unnecessary dependencies being installed too.

jvlmdr commented 4 years ago

Hi! Thanks for pointing this out. Yeah, maybe we should remove these flake dependencies from requirements.txt since they are only required for development, not for a typical user. We could add pip install flake8 to precommit.sh, however I think that behaviour might be a little too surprising, so we can just put a comment or a test here. What do you think @cheind ?

Should we keep pytest and pytest-benchmark in requirements.txt, or remove these too?

cheind commented 4 years ago

Hey! So yes, flake8 and related (also pytest) should be removed from requirements.txt.

For developers, we could add a dev_requirements.txt or similar which needs to be installed with pip install -r dev_requirements.txt. Or even better, we utilize extras_require from setup.py as explained here.

jvlmdr commented 4 years ago

I have made this change on the develop branch. Travis and appveyor builds passing.

I didn't look into the extras_require yet.

cheind commented 4 years ago

Thanks @jvlmdr. Just noticed the the pre-commit script for the first time :) That's supposed to be run manually, right? Or is this some kind of git-hook?

jvlmdr commented 4 years ago

Yep, I thought it might be too annoying to run it automatically, it's just something that we can ask people to run and ensure that it passes before merging a PR.

cheind commented 4 years ago

Good idea!

IRDonch commented 4 years ago

Thanks for the quick response. FWIW, if you've removed pytest, you might also want to remove motmetrics.tests from packages in setup.py.

cheind commented 4 years ago

@IRDonch is that something we should worry about? After all, people could still be running the tests (pytest --pyargs motmetrics) if they have pytest installed? We are basically following the guidelines here.

IRDonch commented 4 years ago

@cheind It's a similar concern to what the original problem was - you're installing code that is unnecessary for most users. That said, I don't mind it that much, so if you like it the way it is, that's fine.

The original problem's been resolved, so I'm closing the issue.