Add exception for missing system requirements #281

kamalesh0406 · 2021-12-25T18:11:57Z

Pre-requisites

Please ensure you have done the following:

I have read the CONTRIBUTING.md document.
If my change requires a change to the documentation, I have updated the documentation accordingly.
I have added tests to cover my changes.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Describe changes

Briefly describe the changes you have introduced.

I have modified the check_installation function and the Integration class to look for missing system requirements particularly pertaining to the graphviz integration. The new code raises an Exception during zenml init were it mentions the library whose package is missing from the system. I tried to make it raise the error during zenml example run but for that to be done the changes should be done in example cli and it was mentioned in the following issue #266 that an override for check_installation is preferable.

CLAassistant · 2021-12-25T18:12:02Z

All committers have signed the CLA.

AlexejPenner

Overall, the code changes look good to me. I’m away from my machine so I can’t verify functionality right now. I added a small change request in the comments regarding the error message.

Thanks a lot for your contribution Kamalesh☺️

AlexejPenner · 2021-12-25T18:37:07Z

src/zenml/integrations/integration.py

+                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}"


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.”

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.

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.

Yup, I will do that.

michael-zenml · 2021-12-25T19:07:26Z

I like the implementation using the additional dict of system requirements! We might consider using shutil.which(...) to check if a command exists without actually running it using subprocess.run(...), which would allow us to simplify the system requirements to ["dot"] (without the -V).

kamalesh0406 · 2021-12-25T19:42:58Z

I like the implementation using the additional dict of system requirements! We might consider using shutil.which(...) to check if a command exists without actually running it using subprocess.run(...), which would allow us to simplify the system requirements to ["dot"] (without the -V).

Thank you for the suggestion, I will change the subprocess to shutil.which .

AlexejPenner · 2021-12-25T20:41:51Z

Makes sense that we have failing tests now, as they also have a dependency on check_installation. For the scope of this bugfix it might make sense to hard code the installation of graphviz on the level of our github actions. In the long run we could then maybe have a feature ticket to implement this in a more generic way for all possible future “System requirements” as well.

Do you need any additional guidance regarding our github actions?

kamalesh0406 · 2021-12-25T21:01:10Z

Makes sense that we have failing tests now, as they also have a dependency on check_installation. For the scope of this bugfix it might make sense to hard code the installation of graphviz on the level of our github actions. In the long run we could then maybe have a feature ticket to implement this in a more generic way for all possible future “System requirements” as well.

Do you need any additional guidance regarding our github actions?

So the idea is to now ensure that graphviz is installed on the system that the github actions is testing? Please do correct me if I am wrong.

kamalesh0406 · 2021-12-25T21:54:03Z

I have modified the error message, subprocess library and the github actions as well in the new commit.

AlexejPenner · 2021-12-25T23:37:52Z

I have modified the error message, subprocess library and the github actions as well in the new commit.

Thanks Kamalesh. I don’t see any changes on the github actions within your commit. I think you may have forgotten to commit or push your changes within the .github folder?

kamalesh0406 · 2021-12-26T00:01:03Z

I have modified the error message, subprocess library and the github actions as well in the new commit.

Thanks Kamalesh. I don’t see any changes on the github actions within your commit. I think you may have forgotten to commit or push your changes within the .github folder?

Yup sorry I forgot to push them. I have modified the commit and pushed it again.

AlexejPenner · 2021-12-26T00:15:38Z

.github/workflows/pull_request.yml

@@ -24,6 +24,14 @@ jobs:
        with:
          python-version: ${{ matrix.python-version }}

+      - name: Install System Dependencies


I believe the correct syntax here would be to name each condition separately:

- name: condition no 1 if: condition_1=true run: script for condition 1 - name: condition 2 if: condition_2=true run: script for condition 2

Okay, that makes sense. I have pushed the changes, sorry for the inconvenience.

The test failed since I used double quotes instead of using single quotes for specifying the system.

Thanks for all your efforts. Looks good to me so far 😊

Are you able to see the logs of the Github actions? There seem to be some linting issues. If you have no access I can paste the logs here ☺️

Yup I have modified my code according to the error. :)

htahir1 · 2021-12-27T06:14:31Z

@kamalesh0406 In order to fix these linting issues, we have a script in the scripts folder called format.sh. Run it as follows:

cd zenml/
poetry run bash scripts/format.sh

or simply:

./scripts/format.sh

You can also run linting as follows:

poetry run bash scripts/lint.sh
poetry run bash scripts/check-spelling.sh

And tests as follows:

poetry run bash scripts/test-coverage-xml.sh

I'll add this to the CONTRIBUTING.md for future reference

src/zenml/integrations/integration.py

AlexejPenner · 2021-12-27T16:44:16Z

@kamalesh0406 Sorry for pushing directly onto your repo, I must have pressed the wrong button, was just gonna add it as a suggestion. I think now there's just a newline missing at the end of the file before we can merge

kamalesh0406 · 2021-12-27T17:24:41Z

Okay I will check it and push the changes.

kamalesh0406 · 2021-12-27T18:31:11Z

I am not able to recreate the linting issue on my local, can you specify the exact issue? The github actions output is not very clear as well.

AlexejPenner · 2021-12-27T21:34:12Z

I am not able to recreate the linting issue on my local, can you specify the exact issue? The github actions output is not very clear as well.

Locally within your repo you should be able to just run poetry run bash scripts/format.sh - this will fix the issue for you. Then you just need to commit and push the file.

And yeah, I agree the logs in the Github actions could be a bit more specific. In this case the file src/zenml/integrations/integration.py is missing a newline at the end.

AlexejPenner · 2021-12-28T09:39:19Z

@kamalesh0406 Looks great, thanks for your contribution 🙏

Add exception for missing system requirements

e20cdba

AlexejPenner requested changes Dec 25, 2021

View reviewed changes

kamalesh0406 force-pushed the add_graphviz_system_error branch from f5ef0a2 to e20cdba Compare December 25, 2021 23:49

Modify error message and add graphviz install to github actions

899ca2c

AlexejPenner requested changes Dec 26, 2021

View reviewed changes

kamalesh0406 force-pushed the add_graphviz_system_error branch from 918d50a to 899ca2c Compare December 26, 2021 03:52

Update github actions for pull request

1fc168d

AlexejPenner approved these changes Dec 26, 2021

View reviewed changes

Modify code for linter test

c120467

AlexejPenner reviewed Dec 27, 2021

View reviewed changes

src/zenml/integrations/integration.py Outdated Show resolved Hide resolved

Different order for imports

ffbbad5

Add whitespace to correct lint issue

950297d

AlexejPenner merged commit f107622 into zenml-io:main Dec 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add exception for missing system requirements #281

Add exception for missing system requirements #281

kamalesh0406 commented Dec 25, 2021

CLAassistant commented Dec 25, 2021 •

edited

AlexejPenner left a comment

AlexejPenner Dec 25, 2021

kamalesh0406 Dec 25, 2021

AlexejPenner Dec 25, 2021

kamalesh0406 Dec 25, 2021

michael-zenml commented Dec 25, 2021 •

edited

kamalesh0406 commented Dec 25, 2021

AlexejPenner commented Dec 25, 2021

kamalesh0406 commented Dec 25, 2021

kamalesh0406 commented Dec 25, 2021

AlexejPenner commented Dec 25, 2021

kamalesh0406 commented Dec 26, 2021

AlexejPenner Dec 26, 2021

kamalesh0406 Dec 26, 2021

kamalesh0406 Dec 26, 2021

AlexejPenner Dec 26, 2021

kamalesh0406 Dec 26, 2021

htahir1 commented Dec 27, 2021

AlexejPenner commented Dec 27, 2021

kamalesh0406 commented Dec 27, 2021

kamalesh0406 commented Dec 27, 2021 •

edited

AlexejPenner commented Dec 27, 2021 •

edited

AlexejPenner commented Dec 28, 2021

Add exception for missing system requirements #281

Add exception for missing system requirements #281

Conversation

kamalesh0406 commented Dec 25, 2021

Pre-requisites

Types of changes

Describe changes

CLAassistant commented Dec 25, 2021 • edited

AlexejPenner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-zenml commented Dec 25, 2021 • edited

kamalesh0406 commented Dec 25, 2021

AlexejPenner commented Dec 25, 2021

kamalesh0406 commented Dec 25, 2021

kamalesh0406 commented Dec 25, 2021

AlexejPenner commented Dec 25, 2021

kamalesh0406 commented Dec 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htahir1 commented Dec 27, 2021

AlexejPenner commented Dec 27, 2021

kamalesh0406 commented Dec 27, 2021

kamalesh0406 commented Dec 27, 2021 • edited

AlexejPenner commented Dec 27, 2021 • edited

AlexejPenner commented Dec 28, 2021

CLAassistant commented Dec 25, 2021 •

edited

michael-zenml commented Dec 25, 2021 •

edited

kamalesh0406 commented Dec 27, 2021 •

edited

AlexejPenner commented Dec 27, 2021 •

edited