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.

Sign up for GitHub

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

Jump to bottom

Add exception for missing system requirements #281

Merged
merged 6 commits into from
Dec 28, 2021
Merged

Add exception for missing system requirements #281

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e20cdba
Add exception for missing system requirements
kamalesh0406 Dec 25, 2021
899ca2c
Modify error message and add graphviz install to github actions
kamalesh0406 Dec 25, 2021
1fc168d
Update github actions for pull request
kamalesh0406 Dec 26, 2021
c120467
Modify code for linter test
kamalesh0406 Dec 26, 2021
ffbbad5
Different order for imports
AlexejPenner Dec 27, 2021
950297d
Add whitespace to correct lint issue
kamalesh0406 Dec 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/zenml/integrations/graphviz/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@ class GraphvizIntegration(Integration):

NAME = GRAPHVIZ
REQUIREMENTS = ["graphviz>=0.17"]

SYSTEM_REQUIREMENTS = {"graphviz":"dot -V"}

GraphvizIntegration.check_installation()
11 changes: 11 additions & 0 deletions src/zenml/integrations/integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
from typing import Any, Dict, List, Tuple, Type, cast

import pkg_resources
import subprocess

from zenml.integrations.registry import integration_registry
from zenml.logger import get_logger
from zenml.exceptions import DoesNotExistException

logger = get_logger(__name__)

Expand All @@ -42,10 +44,19 @@ class Integration(metaclass=IntegrationMeta):

REQUIREMENTS: List[str] = []

SYSTEM_REQUIREMENTS: Dict[str, str] = {}

@classmethod
def check_installation(cls) -> bool:
"""Method to check whether the required packages are installed"""
try:
for req, command in cls.SYSTEM_REQUIREMENTS.items():
result = subprocess.run(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True, shell=True)
if result.returncode!=0:
raise DoesNotExistException(
f"Unable to find linux/unix installation of {req}"
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion this message should be os - agnostic. Maybe also add a sentence on what the user should do to fix this. Maybe a second sentence like: “Please install the package and retry.”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, even I was thinking about this. I will try to use platform to print a more OS-specific message. The only issue is platform.system() prints 'Darwin' for macOS and that might confuse the user at times.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternativly, you could also just not mention the platform at all. The user already knows his platform and can find the appropriate way to install the package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I will do that.

)

for r in cls.REQUIREMENTS:
pkg_resources.get_distribution(r)
logger.debug(
Expand Down