Conversation
|
@TMVKasiViswanath all tests seem to be failing. Could you give it a look? |
harshad16
left a comment
harshad16
left a comment
There was a problem hiding this comment.
great initiative
getattr in init.py might increase the maintainability of the module , right ?
I'm also not able to follow, how this fixes the #225 request.
as the request there is to, split the module with main dependency and optional dependency.
The current installation of prometheus_api_client bring in pandas and additional package that is only required for Metrics class usage but not for PrometheusConnect operation.
For fixing the issue, we would need to change the packaging of the module.
| __title__ = "prometheus-connect" | ||
| __version__ = "0.5.7" | ||
|
|
||
| from .prometheus_connect import * # noqa F403 |
There was a problem hiding this comment.
The additional classes also need to be adjusted, if we are explicitly defining each class available with prometheus_api_client.
from prometheus_api_client import PrometheusApiClientException
this would fails with current implementation.
There was a problem hiding this comment.
yes, I forgot to add 2 more imports in the init file
Yes I have made the changes and made the new commit to it |
|
@4n4nd The test failures are not caused by my changes. The errors indicate that demo.robustperception.io:9090 is unreachable ([Errno 101] Network is unreachable). To verify, I set up a local Prometheus instance, pointed PrometheusConnect to it, and ran the tests. Everything passed successfully.(All the 44 test cases in the tests folder got executed) This suggests that the issue is with the availability of the demo server, not my modifications to init.py. Once the server is back up, rerunning the workflow should make the tests pass without any issues. |
|
@TMVKasiViswanath that should be fixed now. Could you please rebase and try again? |
There was a problem hiding this comment.
Pull request overview
This PR implements lazy imports in the prometheus_api_client package's __init__.py file using the __getattr__ pattern. Instead of importing all classes at module load time, classes are now imported only when they are first accessed, which reduces initial memory footprint and import time. The PR replaces the previous wildcard import (from .prometheus_connect import *) and explicit imports with a dynamic __getattr__ function that handles imports on-demand.
- Converts eager imports to lazy imports using
__getattr__for main classes (PrometheusConnect, Metric, MetricsList, MetricSnapshotDataFrame, MetricRangeDataFrame) - Keeps exception classes (PrometheusApiClientException, MetricValueConversionError) as eager imports
- Removes wildcard import pattern in favor of explicit lazy loading
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from .metrics_list import MetricsList # noqa F401 | ||
| from .metric_snapshot_df import MetricSnapshotDataFrame # noqa F401 | ||
| from .metric_range_df import MetricRangeDataFrame # noqa F401 | ||
| from .exceptions import PrometheusApiClientException, MetricValueConversionError |
There was a problem hiding this comment.
[nitpick] Exception classes should also be lazy-loaded for consistency. Currently PrometheusApiClientException and MetricValueConversionError are eagerly imported on line 6, which defeats part of the purpose of this optimization. These exceptions should be added to the __getattr__ function like the other classes, and only imported when actually used.
| from .metrics_list import MetricsList # noqa F401 | ||
| from .metric_snapshot_df import MetricSnapshotDataFrame # noqa F401 | ||
| from .metric_range_df import MetricRangeDataFrame # noqa F401 | ||
| from .exceptions import PrometheusApiClientException, MetricValueConversionError |
There was a problem hiding this comment.
Missing blank line between imports and function definition. PEP 8 style guide recommends two blank lines before top-level function definitions to improve readability.
| from .exceptions import PrometheusApiClientException, MetricValueConversionError | |
| from .exceptions import PrometheusApiClientException, MetricValueConversionError |
| def __getattr__(name): | ||
| if name == "PrometheusConnect": | ||
| from .prometheus_connect import PrometheusConnect | ||
| return PrometheusConnect | ||
| elif name == "Metric": | ||
| from .metric import Metric | ||
| return Metric | ||
| elif name == "MetricsList": | ||
| from .metrics_list import MetricsList | ||
| return MetricsList | ||
| elif name == "MetricSnapshotDataFrame": | ||
| from .metric_snapshot_df import MetricSnapshotDataFrame | ||
| return MetricSnapshotDataFrame | ||
| elif name == "MetricRangeDataFrame": | ||
| from .metric_range_df import MetricRangeDataFrame | ||
| return MetricRangeDataFrame | ||
| raise AttributeError(f"module {__name__} has no attribute {name}") |
There was a problem hiding this comment.
Missing __all__ definition for lazy-loaded modules. When implementing lazy imports via __getattr__, it's a best practice to also define __all__ to explicitly declare the public API. This ensures tools like type checkers, IDEs, and from module import * work correctly. Add: __all__ = ["PrometheusConnect", "Metric", "MetricsList", "MetricSnapshotDataFrame", "MetricRangeDataFrame", "PrometheusApiClientException", "MetricValueConversionError"]
| elif name == "MetricRangeDataFrame": | ||
| from .metric_range_df import MetricRangeDataFrame | ||
| return MetricRangeDataFrame | ||
| raise AttributeError(f"module {__name__} has no attribute {name}") |
There was a problem hiding this comment.
The error message formatting uses an f-string but doesn't include quotes around the attribute name, making it inconsistent with Python's standard AttributeError format. The standard format is "module 'module_name' has no attribute 'attr_name'" (note the quotes around both module and attribute names). Change to: raise AttributeError(f"module '{__name__}' has no attribute '{name}'")
| raise AttributeError(f"module {__name__} has no attribute {name}") | |
| raise AttributeError(f"module '{__name__}' has no attribute '{name}'") |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
3 participants
Optimized imports in the init method by implementing lazy imports. This prevents unnecessary imports of unused classes, ensuring that only the required classes are imported when needed. This reduces memory usage and improves efficiency. I have tested this to confirm that the changes work as expected.