Refactor requirements

[mrwbarg]

Closes #393.
Organizing dependencies.

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (37c8a3a) 78.64% compared to head (2abb3f9) 78.64%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #407   +/-   ##
=======================================
  Coverage   78.64%   78.64%           
=======================================
  Files          73       73           
  Lines        3152     3152           
  Branches      528      528           
=======================================
  Hits         2479     2479           
  Misses        598      598           
  Partials       75       75           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[VMRuiz]

I'm good with the current approach but I think it was suggested to get rid of requirements.txt files and just leave pre-commit & tox to fetch all dependencies from the setup.py file.

Some examples:

• https://github.com/scrapy/scrapy/blob/master/tox.ini
• https://github.com/scrapy-plugins/scrapy-zyte-api/blob/main/tox.ini

[VMRuiz]

Could you also please update https://github.com/scrapinghub/spidermon/blob/master/CONTRIBUTING.rst to replace references to requirements.txt files and substitute it with just installing tox ?

[VMRuiz]

We need to review the list of packages on setup.py as not all are required by spidermon itself

[kmike]

It can be a good practice to pin the package versions here.

[kmike]

Would it make sense to move packags like slack_sdk, sentry_sdk and boto here? Are they optional dependencies of spidermon, or are they all required for spidermon to work?

[VMRuiz]

They are not required by Spidermon itself but by different actions:

• boto is required by actions/reports/s3.py
• boto3 is required by actions/email/ses.py
• sentry_sdk is required by actions/sentry/__init__.py
• slack_sdk is required by actions/slack/__init__.py

Before this change, most-common user case of spidermon was to install everything with spidermon[monitoring]. The idea was to make this the default installing by moving all the packages into install_requires and keep monitoring empty to be backward compatible.

Alternatively, we could move this packages into a new section named actions but that wouldn't be backwards compatible and may be quite problematic for most people.

[wRAR]

What about the reqs that were moved from tests_require / spidermon[tests] to install_requires?

[VMRuiz]

If I'm not mistaken those would be:

• itemadapter: [contrib/scrapy/pipelines.py], [contrib/scrapy/extensions.py],
• requests [contrib/actions/telegram/init.py],[contrib/actions/discord/init.py]:
• scrapinghub-entrypoint-scrapy (sh_scrapy) [contrib/stats/statscollectors/sc_collections.py]
• twisted [contrib/scrapy/extensions.py]

itemadapter, requests and sh_scrapy are required by different parts of the code. I guess they were not previously added by mistake, because they are already present on most Scrapy stacks anyway.

The only package that seems redundant to me is twisted which I think we should drop as is already covered by scrapy.

[kmike]

I assume the dependencies like boto or slack-sdk are all optional, and most users don't use all of them.

Not installing them by default minimizes scenarios when some of these packages don't install for some reason for the users - e.g. a package drops support for some Python version which spidermon still supports (and which an user is using), or requirements of some package conflict with requirements which user's project has.

If these packages are not optional, then in such scenario spidemon installation is broken for users who don't even use these features.

That said, if based on your experience with these particular packages you found it unlikely they break, it looks fine to install them by default.

So, I'm approving the PR, and let you folks decide if the concern is worth addressing, or if addressing it just complicates things.

If it's something which should be addressed, probably the easiest way to go is to move all optional requirements to some section like pip install spidermon[contrib] or pip install spidermon[all], keep only the required ones in default install_requires, and also document pip install spidermon[all] as the way to install spidermon.

[VMRuiz]

I'm worried about adding a new optional requirements like contrib all all after we already have monitors in place that is used by default by most users.

[VMRuiz]

I have updated the PR with some changes:

• Remove optional dependencies from install_requires and put them as extras
• Keep extra monitoring naming for backward compatibility. We can refactor this later to all or contrib if needed.
• Split tox configuration between testenv (all tests + all extras) and testenv:base (core tests + no extras except Scrapy and Jinja2). The reason for adding Scrapy and Jinja2 to testenv:base is because default conftest.py and others top-level tests contains dependencies on them. We should fix that in a following PR.