make matplotlib optional #303
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to make matplotlib an optional dependency to reduce unnecessary dependencies for users who don't need plotting functionality. However, the implementation uses a non-standard approach by changing the package name rather than using Python's extras_require mechanism.
Key Changes
- Removed matplotlib from requirements.txt to make it non-mandatory
- Changed package name from
prometheus-api-clienttoprometheus-api-client-optional-matplotlibin setup.py and init.py - Updated error message in metric_plot.py to guide users on installing matplotlib when needed
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| setup.py | Changed package name and description, but missing extras_require configuration for optional matplotlib dependency |
| requirements.txt | Removed matplotlib from required dependencies |
| prometheus_api_client/metric_plot.py | Improved error message to guide users on installing matplotlib, but references incorrect package name |
| prometheus_api_client/init.py | Updated __title__ to match new package name, deviating from standard practices |
Note: The current approach creates a fork of the package rather than properly making matplotlib optional. The standard Python approach would be to keep the package name as prometheus-api-client and use extras_require={'plotting': ['matplotlib']} in setup.py, allowing users to install with pip install prometheus-api-client[plotting] when they need plotting functionality.
Comments suppressed due to low confidence (1)
setup.py:47
- The setup.py is missing the
extras_requireconfiguration to make matplotlib optional. To properly support optional dependencies, you should add:
setuptools.setup(
# ... other parameters ...
install_requires=get_install_requires(),
extras_require={
'plotting': ['matplotlib'],
},
# ... remaining parameters ...
)This allows users to install with pip install prometheus-api-client[plotting] to get matplotlib support, which matches the error message guidance in metric_plot.py.
setuptools.setup(
name="prometheus-api-client-optional-matplotlib",
version=VERSION,
author="Anand Sanmukhani",
author_email="[email protected]",
description="A Python API client for Prometheus with optional matplotlib dependency",
long_description=long_description,
long_description_content_type="text/markdown",
url="https://github.com/4n4nd/prometheus-api-client-python",
install_requires=get_install_requires(),
packages=setuptools.find_packages(),
package_data={"prometheus-api-client": ["py.typed"]},
tests_require=["httmock"],
classifiers=[
"Programming Language :: Python :: 3.13",
"License :: OSI Approved :: MIT License",
"Operating System :: OS Independent",
],
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raise ImportError("matplotlib was not found") | ||
| raise ImportError( | ||
| "matplotlib is required for plotting functionality. " | ||
| "Install with: pip install 'prometheus-api-client[plotting]'" |
There was a problem hiding this comment.
The error message references prometheus-api-client[plotting] but the package name in setup.py has been changed to prometheus-api-client-optional-matplotlib. This creates an inconsistency.
If you intend to publish this as a different package, the installation instruction should be updated to match. However, it would be better to keep the original package name and use extras_require to make matplotlib optional, allowing users to install with pip install prometheus-api-client[plotting] as the error message suggests.
| "Install with: pip install 'prometheus-api-client[plotting]'" | |
| "Install with: pip install 'prometheus-api-client-optional-matplotlib'" |
| VERSION = get_version() | ||
| setuptools.setup( | ||
| name="prometheus-api-client", | ||
| name="prometheus-api-client-optional-matplotlib", |
There was a problem hiding this comment.
The package name change from prometheus-api-client to prometheus-api-client-optional-matplotlib is inconsistent with the typical approach for making dependencies optional. The standard Python packaging practice is to keep the package name the same and use extras_require in setup.py to provide optional features.
Changing the package name creates a fork of the project rather than making matplotlib optional in the existing package. Consider keeping the original name prometheus-api-client and using extras_require={'plotting': ['matplotlib']} in setup.py instead.
| """A collection of tools to collect and manipulate prometheus metrics.""" | ||
|
|
||
| __title__ = "prometheus-connect" | ||
| __title__ = "prometheus-api-client-optional-matplotlib" |
There was a problem hiding this comment.
The __title__ should remain as prometheus-api-client (or the original value prometheus-connect if that was intentional). Changing it to prometheus-api-client-optional-matplotlib is inconsistent with standard Python packaging practices for optional dependencies. The package title typically doesn't reflect which dependencies are optional.
| __title__ = "prometheus-api-client-optional-matplotlib" | |
| __title__ = "prometheus-api-client" |
|
superseded by #309 |
|
@4n4nd thank you, super excited to see this wil be officially supported! closing my own PR |
2 participants
Fixes #225
This is a breaking change for users, but it solves a major painpoint for end users who don't want to pull in unecessary dependencies.
I've pushed a version of the package to pypi at https://pypi.org/project/prometheus-api-client-optional-matplotlib/ which is a drop-in replacement for prometheus-api-client without the matplotlib dependency.