Skip to content

Resolved issue #225 #293

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TMVKasiViswanath
Copy link

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.

@4n4nd
Copy link
Owner

4n4nd commented Feb 21, 2025

@TMVKasiViswanath all tests seem to be failing. Could you give it a look?

Copy link
Collaborator

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""A collection of tools to collect and manipulate prometheus metrics."""

__title__ = "prometheus-connect"
__version__ = "0.5.7"

from .prometheus_connect import * # noqa F403
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I forgot to add 2 more imports in the init file

@TMVKasiViswanath
Copy link
Author

@TMVKasiViswanath all tests seem to be failing. Could you give it a look?

Yes I have made the changes and made the new commit to it

@TMVKasiViswanath TMVKasiViswanath marked this pull request as draft February 21, 2025 20:40
@TMVKasiViswanath TMVKasiViswanath marked this pull request as ready for review February 21, 2025 20:40
@TMVKasiViswanath
Copy link
Author

TMVKasiViswanath commented Feb 22, 2025

@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.
Test Passed on Local Prometheus

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 this pull request may close these issues.

3 participants