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

Separate and cleanup requirements #82

Closed
cdce8p opened this issue Jan 26, 2022 · 2 comments · Fixed by #85
Closed

Separate and cleanup requirements #82

cdce8p opened this issue Jan 26, 2022 · 2 comments · Fixed by #85

Comments

@cdce8p
Copy link

cdce8p commented Jan 26, 2022

While working on the Home-Assistant dependency bump, I've been wondering if it might be worth it to separate and cleanup the requirement a bit. This would definitely help reduce dependency conflicts but only installing what is truly needed.

At the moment all dependencies are specified in requirements.txt. This leads to a situation where amqtt needs to be installed for Home Assistant, and subsequently creates dependency conflicts, although it isn't even needed there. I would like to suggest splitting the dependencies into what is required for everything (these should remain as install_requires), and what is needed for the mqtt server. Those could be moved to extras_require.

The only downside would probably be that users would need to use pip install volvooncall[mqtt] instead if they want like to use the mqtt server.

Furthermore, I would recommend against reading the requirements from requirements.txt in setup.py. requirements.txt is still needed (for CI and testing) and thus should include all dependencies. Especially with the split, it would however be better to specify the dependencies independently in setup.cfg.

Last, I know I just recommended updating the websockets dependency. While checking the requirement, I just found that it isn't even used directly. So it might be safe to remove entirely? If other packages depend on it, they specify a dependency range for it themselves (eg. amqtt).

https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#optional-dependencies

home-assistant/core#64942

/CC @decompil3d

@frenck
Copy link

frenck commented Feb 17, 2022

Yup, bumping to 0.9.2 resolves the websocket conflict for Home Assistant, but introduces:

amqtt 0.10.0 has requirement PyYAML<6.0.0,>=5.4.0, but you have pyyaml 6.0

Even though Home Assistant doesn't even need amqtt in the first place. Would be nice if it was specified as an extra requirement as listed above by @cdce8p.

Anyways: This currently prevents bumping the Volvo On Call integration, but also blocks us from upgrading to the latest pip resolver. Once we upgrade to the latest pip that forces us to use the new resolver (which is expected to land soon), we have no choice but to disable the Volvo On Call integration.

If we can do anything to help, feel free to reach out!

../Frenck

/CC @decompil3d

@cdce8p
Copy link
Author

cdce8p commented Feb 17, 2022

@frenck I've already provided a PR for amqtt to update the PyYAML requirement. Just waiting for a new release there. You're right though that separating the requirements would be helpful for the future.

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 a pull request may close this issue.

2 participants