redis-om-python icon indicating copy to clipboard operation
redis-om-python copied to clipboard

Clean dependencies

Open moznuy opened this issue 2 years ago • 4 comments

Fixes #234

Changes

  • [x] Clean dependencies:
    • Make hiredis, email-validator optional for public and required for dev dependencies.
    • Add hiredis, email-validator to package extras.
    • Remove pptree, cleo, six from public dependencies.
    • Remove ipdb, pylint from dev dependencies.
    • Move types-redis, types-six to dev dependencies.
    • Change RedisModel.from_redis to Python 3 syntax for six removal.
  • [x] Add documentation about optional dependencies.
  • [x] Run make lint format test.
  • [ ] Run poetry update?
  • [ ] Sort dependencies?
  • [ ] Remove types-six? (six is removed)
  • [ ] Is pytest-xdist needed?
  • [ ] Is tox(with tox-env) needed?

Comparison of dist/<>/setup.py:

Before

install_requires = [
    "aioredis>=2.0.0,<3.0.0",
    "cleo==1.0.0a4",
    "click>=8.0.1,<9.0.0",
    "hiredis>=2.0.0,<3.0.0",
    "pptree>=3.1,<4.0",
    "pydantic>=1.8.2,<2.0.0",
    "python-ulid>=1.0.3,<2.0.0",
    "redis>=3.5.3,<5.0.0",
    "six>=1.16.0,<2.0.0",
    "types-redis>=3.5.9,<5.0.0",
    "types-six>=1.16.1,<2.0.0",
    "typing-extensions>=4.0.0,<5.0.0",
]

After

install_requires = [
    "aioredis>=2.0.0,<3.0.0",
    "click>=8.0.1,<9.0.0",
    "pydantic>=1.8.2,<2.0.0",
    "python-ulid>=1.0.3,<2.0.0",
    "redis>=3.5.3,<5.0.0",
    "typing-extensions>=4.0.0,<5.0.0",
]

extras_require = {
    "email": ["email-validator>=1.2.1,<2.0.0"],
    "hiredis": ["hiredis>=2.0.0,<3.0.0"],
}

moznuy avatar May 06 '22 17:05 moznuy

Any input will be appreciated

moznuy avatar May 12 '22 19:05 moznuy

Are there any updates on this? #233 has been merged, which means that aioredis can be removed here as well.

As for the outstanding todo tasks on this PR it seems that they mostly are referring to dev dependencies (questions related to tox, pytest and even sorting dependencies).

I'd love to see at least the removal of aioredis released soon as it's not a dependency that is used anymore and I'd hate to see it installed when using this package when it's not used anywhere.

Not sure if I could contribute in any way to help get this in, but as for the questions I could provide some info for:


Remove types-six? (six is removed)

I believe this makes sense to remove, seeing as six isn't being used in the package anymore.

Is pytest-xdist needed?

This package provides concurrency to tests being run, which is used in both test and test_oss make commands. pytest -n auto tells pytest ot run with 'auto' number of workers (count depends on system running the tests).

Is tox(with tox-env) needed?

It doesn't look like tox is being used? CI runs on Github Actions with a matrix over different Python versions, though it might still be useful to run manually before it hits CI. It's not documented anywhere though.


Poetry 1.2+ (IIRC) introduces dependency groups instead of just main + dev, which might make sense to utilise for optional local development stuff like this. Or at least have some way of just optionally installing these local development tools. I guess dev dependencies can be marked as optional, just as main dependencies can be?

ikornaselur avatar Nov 21 '22 14:11 ikornaselur

Are there any updates on this? https://github.com/redis/redis-om-python/pull/233 has been merged, which means that aioredis can be removed here as well.

The problem was/is that the communication between maintainers and contributors was non existent (maybe something has changed). I would glad to come back and rebase onto HEAD / finish PR, if there is a chance this might be merged.

This package provides concurrency to tests being run, which is used in both test and test_oss make commands. pytest -n auto tells pytest ot run with 'auto' number of workers (count depends on system running the tests).

As far as I remember, it was faster for me to run tests without concurrency than with it. The question is "is it really needed?", as stated in the #234.

moznuy avatar Nov 21 '22 16:11 moznuy

As far as I remember, it was faster for me to run tests without concurrency than with it. The question is "is it really needed?", as stated in the https://github.com/redis/redis-om-python/issues/234.

Fair enough! Hadn't noticed that point in #234 - the test suite seems to be fairly quick already, just a minute for the suite if looking at the CI run, so personally I don't really see the use of xdist.. especially if it's slower in practice! :smile:

The problem was/is that the communication between maintainers and contributors was non existent (maybe something has changed). I would glad to come back and rebase onto HEAD / finish PR, if there is a chance this might be merged.

I was wondering about this yesterday, if there was any public place for discussion outside of just GitHub issues, but couldn't find any at a cursory glance. It's definitely frustrating to have things in limbo, so hopefully communication will improve :pray:

ikornaselur avatar Nov 22 '22 10:11 ikornaselur