Made numpy requirement optional. Fixes #4

Efrony commented 4 years ago

Hi! I'm using your new integration in Home assistant. But I can't find how to open the SETTING button in the file endpoints.py. Sorry that write here. I don't know where else to write =)

bendavid commented 4 years ago

I'm happy with this (but it needs to be rebased)

wolph commented 4 years ago

I’ve reapplied the patch :)

bendavid commented 4 years ago

The assert for missing numpy should be consistently added to all the relevant functions. Also please fix the style checks (suggest to use pre-commit hooks as described in the documentation)

wolph commented 4 years ago

I don't think the pre-commit stuff is working but I might be doing something wrong. The usage isn't really clear to me

# python3 .git/hooks/pre-commit
Trim Trailing Whitespace.............................(no files to check)Skipped
Fix End of Files.....................................(no files to check)Skipped
Check docstring is first.............................(no files to check)Skipped
Check Yaml...........................................(no files to check)Skipped
Debug Statements (Python)............................(no files to check)Skipped
Tests should end in _test.py.........................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
black................................................(no files to check)Skipped
pyupgrade............................................(no files to check)Skipped
isort................................................(no files to check)Skipped

# pre-commit run
Trim Trailing Whitespace.............................(no files to check)Skipped
Fix End of Files.....................................(no files to check)Skipped
Check docstring is first.............................(no files to check)Skipped
Check Yaml...........................................(no files to check)Skipped
Debug Statements (Python)............................(no files to check)Skipped
Tests should end in _test.py.........................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
black................................................(no files to check)Skipped
pyupgrade............................................(no files to check)Skipped
isort................................................(no files to check)Skipped

I've also tried to look at the Azure pipelines output but it's not telling me anything useful either: https://dev.azure.com/joshbendavid/aiopylgtv/_build/results?buildId=9&view=logs&jobId=00b6a206-eb91-585b-3d14-cf1a4d7b1970&j=00b6a206-eb91-585b-3d14-cf1a4d7b1970&t=9a9400f1-b707-5d54-e2fd-4fe1d8bc6dda

Feel free to add extra numpy checks to the other methods though. For the time being I've simply added a mock numpy which works fine for my case.

ghost commented 3 years ago

bumping this as I've just got a new LG TV and would like to use this library but not install numpy.

cdce8p commented 3 years ago

I'm also looking forward to this feature. How can I help to get over this finish line?

@WoLpH I could provide a patch to your feature branch if it would help. @bendavid if this PR is stale I could also open a new one with these changes and the remaining fixes.

wolph commented 3 years ago

I've (re-) applied the patch 3 times, I'm just running the fork for now

cdce8p commented 3 years ago

@WoLpH Take a look at https://github.com/WoLpH/aiopylgtv/pull/1 That should resolve the remaining issues. To fix pre-commit, it did:

source venv/bin/activate  # activate virtual env
pip install pre-commit
pre-commit install
pre-commit run --all-files

If you merge the PR into your branch, this PR should be good to go.

wolph commented 3 years ago

Thanks for the help @cdce8p :) I've merged your pull request, let's see if that helps

bendavid commented 3 years ago

I'll come back to this soon, but there are a few other items which are higher priority (and ideally I would like to add some unit tests to systematically make sure this isn't breaking the calibration functionality).

Are there platforms where this is really breaking/preventing to use the library?

wolph commented 3 years ago

With any OS/Python version combination that doesn't have a binary wheel you're in for a very long build time. In my case it is a FreeBSD machine that doesn't come bundled with numpy for the Python version I want. But most pyenv environments will give similar issues.

cdce8p commented 3 years ago

How do we what do move forward with this PR?

I'm using this lib together with Home Assistant on a Raspberry Pi, like a few others I would imagine. Since there is no binary wheel any install or update of numpy takes a really long time during which the whole system is unresponsive.

@bendavid You mentioned that you would like some unit tests to make sure calibration still works as expected. I could contribute those, however since I'm not familiar which this feature I would only assert that numpy is being imported correctly. This however can also be seen just by looking at the changes itself. Let me explain. Below is the output of assert

Python 3.9.1
Type "help", "copyright", "credits" or "license" for more information.
>>> assert np
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'np' is not defined
>>> import numpy as np
>>> assert np
>>>

As you can see, if numpy is imported as expected it just does nothing. Therefore the risk is not if the calibration will continue to work, but if all other features continue to (assuming you don't have numpy installed). Since we've added the asserts only to functions which use or call a function that uses numpy, this shouldn't be an issue.

Please let me / us know how you want to continue.

@WoLpH I just realized that the assert statements are too verbose. It will never print the error message and thus just do more comparisons. I will open a PR on your fork to change those into comments.

cdce8p commented 3 years ago

@WoLpH The PR: https://github.com/WoLpH/aiopylgtv/pull/2

wolph commented 3 years ago

And it's merged :)

Let's hope we can continue now

cdce8p commented 3 years ago

@bendavid Any update?