Fix late-import numpy in stream.core #81790

akx · 2022-11-08T13:59:15Z

Proposed change

numpy is not listed in main requirements and is imported early by conftest.

Import it late to avoid an ImportError.

Split out from #81678.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.
- Nothing new.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

`numpy` is not listed in main requirements and is imported early by `conftest`. Split out from home-assistant#81678 Co-authored-by: uvjustin <46082645+uvjustin@users.noreply.github.com>

home-assistant · 2022-11-08T14:33:35Z

Hey there @hunterjm, @uvjustin, @allenporter, mind taking a look at this pull request as it has been labeled with an integration (stream) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of stream can trigger bot actions by commenting:

@home-assistant close Closes the issue.
@home-assistant rename Awesome new title Change the title of the issue.
@home-assistant reopen Reopen the issue.
@home-assistant unassign stream Removes the current integration label and assignees on the issue, add the integration domain after the command.

homeassistant/components/stream/core.py

uvjustin · 2022-11-08T14:42:07Z

homeassistant/components/stream/core.py

+    if orientation == 8:  # Rotate right
+        return np.rot90(image, -1).copy()
+    return image  # Transforms 0 and 1 (and others not yet defined) are identity


Suggested change

if orientation == 8: # Rotate right

return np.rot90(image, -1).copy()

return image # Transforms 0 and 1 (and others not yet defined) are identity

return np.rot90(image, -1).copy() # Rotate right

I'd rather not have an out-of-range value mean "rotate right" 😁

~~If you don't apply, then you end up with a gap in code coverage, so you'd need a different way to solve that.~~
~~(unless _transform_image is not covered?)~~

Edit: I agree that oritentation 999 shouldn't be "rotate right".

It won't get out of range. It is initialized to 1 and only gets changed via websocket which restricts it to 1 through 8.

That is the only way it's currently called. If code was added that calls it a different way then reviewers should be aware of the behavior - perhaps add a comment?
Even if an unexpected value is encountered, I don't think returning a different rotation on an unexpected value is really any better/worse than returning the original (if we are being anal then we should be raising regardless).
I don't see a good reason to complicate the code here.

That is the only way it's currently called.

My point is that at some point someone might refactor it elsewhere for some other use and start using it without knowing you'd need to validate the orientation argument first, and at that point it becomes "oh that's the weird function that turns your picture sideways if you call it wrong".

Anyway, pushed a change that adds internal validation (in the early-return branch) and gets rid of the last if...

Why is it weird to turn it sideways if it's called wrong? I would argue that's probably even better than passing it unchanged as at least you know something is wrong.
This is an internal function that's used only in stream. If someone tries to use it elsewhere they should read it and figure out how it works.
Perhaps the orientation should be typed as an enum but that's beyond the scope of this PR.
I would really prefer it if you could change it as in my original suggestion.

Why is it weird to turn it sideways if it's called wrong? I would argue that's probably even better than passing it unchanged as at least you know something is wrong.

I would not want to be the future someone debugging why on earth some images are being turned sideways for some reason, and to maybe eventually find out the reason is somehow something is passing 9 to this function.

The principle of least astonishment applies here, too.

If you really strongly do feel that this function should rotate images 90 degrees right when miscalled, I can begrudgingly do that, but that's also not what the old implementation did (which was to raise a KeyError).

I've told you what I think already. This is an internally used function that only gets passed ints from 1 to 8. You've added extra code to account for other values to satisfy some hypothetical future where another caller uses it wrongly.
I think if we're very concerned about that, just add a comment clarifying that it should only be called with 1 through 8 and will default to 8.

uvjustin · 2022-11-08T14:44:06Z

Should we add numpy to the stream requirements?

Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>

frenck · 2022-11-09T08:07:03Z

homeassistant/components/stream/core.py

+    if orientation < 2 or orientation > 8:  # Unused/identity orientations
+        return image
+
+    # Keep import here so that we can import stream integration without installing reqs


So just to be clear, why are we adding this additional complexity?

If it is for the case to "just be able to run tests in an incomplete environment" I would highly vote against this change. We should not build/tailor our codebase to such cases.

Agree with Frenck.

It sounds like a huge waste of energy, time, bandwidth, and disk space to expect all 593 packages listed in requirements_test_all.txt to be installed (in e.g. codespaces, devcontainers, ...) just to be able to test a single component's changes (which is my use case).

This, with #81791 and adding recorder requirements to requirements_test.txt (which happens in #81795) enables being able to do that.

Of course, numpy could just be made a hard dependency for stream (given the import it essentially is) and I could add stream's requirements to requirements_test.txt in #81795, and that would work too...

sounds like a huge waste of energy, time, bandwidth, and disk space to expect all 593 packages listed in requirements_test_all.txt to be installed (in e.g. codespaces, devcontainers, ...) just to be able to test a single component's changes (which is my use case).

Codespaces are pre-built, so that should not be a factor. Anyways, testing a single component and its dependencies depends on what you are testing. Meaning, not installing @ full becomes unpredictable, regardless of these changes. Therefore, I don't think we should add exceptions/complexity to support these cases.

Codespaces are pre-built, so that should not be a factor.

Codespace prebuilds apparently need to be enabled separately for everyone working on a fork. I can't access prebuilds made for home-assistant/core. 😞 It takes some 5 minutes for a codespace to spin up (but happily, apparently, it doesn't install everything from all).

EDIT: Besides, the CI infrastructure will test with all nevertheless...

Anyways, testing a single component and its dependencies depends on what you are testing. Meaning, not installing @ full becomes unpredictable, regardless of these changes.

Hm, isn't this what the strict dependencies/requirements manifest system tries to avoid? Even better, not installing all when working on a single component means you can better notice a dependency that's accidentally missing.

Hm, isn't this what the strict dependencies/requirements manifest system tries to avoid?

No?

The point is: This PR added tailored changes and add complexity that enabled the testing of "some" integrations in a non-fully set-up development environment. To me, this is solving a specific and limited use case, that, above all, is inconsistent (as it will not always work). Therefore, I'm against this change, and I think we should close it for that reason.

../Frenck

Fair. Merging #81841 (which just notes that numpy is a hard requirement for stream, which it is) will close this one.

stream.core imports numpy, so it's a hard requirement for the stream component Closes home-assistant#81790 (supersedes it)

Closes #81790

fix: late-import numpy in stream.core

8453e4b

`numpy` is not listed in main requirements and is imported early by `conftest`. Split out from home-assistant#81678 Co-authored-by: uvjustin <46082645+uvjustin@users.noreply.github.com>

akx requested review from hunterjm, uvjustin and allenporter as code owners November 8, 2022 13:59

akx mentioned this pull request Nov 8, 2022

Late-import some modules required via conftest #81678

Closed

14 tasks

MartinHjelmare changed the title ~~fix: late-import numpy in stream.core~~ Fix late-import numpy in stream.core Nov 8, 2022

epenet added the bugfix label Nov 8, 2022

epenet closed this Nov 8, 2022

epenet reopened this Nov 8, 2022

home-assistant bot added the cla-signed label Nov 8, 2022

epenet added the integration: stream label Nov 8, 2022

home-assistant bot assigned hunterjm, uvjustin and allenporter Nov 8, 2022

home-assistant bot added the Quality Scale: internal label Nov 8, 2022

epenet added small-pr PRs with less than 30 lines. core Quality Scale: internal and removed Quality Scale: internal labels Nov 8, 2022

epenet reviewed Nov 8, 2022

View reviewed changes

homeassistant/components/stream/core.py Outdated Show resolved Hide resolved

uvjustin reviewed Nov 8, 2022

View reviewed changes

homeassistant/components/stream/core.py Show resolved Hide resolved

uvjustin reviewed Nov 8, 2022

View reviewed changes

akx and others added 2 commits November 8, 2022 17:25

Don't import numpy unless really required

49f0055

Update homeassistant/components/stream/core.py

02357cb

Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>

akx requested review from uvjustin and removed request for hunterjm and allenporter November 8, 2022 15:26

epenet mentioned this pull request Nov 8, 2022

Improve type hints in camera #81794

Merged

19 tasks

akx mentioned this pull request Nov 8, 2022

Improve (test) requirements generation #81795

Closed

14 tasks

Remove orientation == 8 condition

ab5a046

frenck reviewed Nov 9, 2022

View reviewed changes

akx added a commit to akx/home-assistant-core that referenced this pull request Nov 9, 2022

stream: Add numpy to requirements

f3c23a2

stream.core imports numpy, so it's a hard requirement for the stream component Closes home-assistant#81790 (supersedes it)

akx mentioned this pull request Nov 9, 2022

Add numpy requirement to stream #81841

Merged

14 tasks

frenck closed this in #81841 Nov 9, 2022

frenck pushed a commit that referenced this pull request Nov 9, 2022

Add numpy requirement to stream (#81841)

7384193

Closes #81790

github-actions bot locked and limited conversation to collaborators Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix late-import numpy in stream.core #81790

Fix late-import numpy in stream.core #81790

akx commented Nov 8, 2022 •

edited by epenet

home-assistant bot commented Nov 8, 2022

uvjustin Nov 8, 2022

uvjustin Nov 8, 2022

akx Nov 8, 2022

epenet Nov 8, 2022 •

edited

uvjustin Nov 8, 2022

uvjustin Nov 8, 2022

akx Nov 8, 2022

uvjustin Nov 8, 2022

akx Nov 8, 2022

uvjustin Nov 9, 2022

uvjustin commented Nov 8, 2022

frenck Nov 9, 2022 •

edited

balloob Nov 9, 2022

akx Nov 9, 2022 •

edited

frenck Nov 9, 2022

akx Nov 9, 2022 •

edited

frenck Nov 9, 2022

akx Nov 9, 2022

Fix late-import numpy in stream.core #81790

Fix late-import numpy in stream.core #81790

Conversation

akx commented Nov 8, 2022 • edited by epenet

Proposed change

Type of change

Checklist

home-assistant bot commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epenet Nov 8, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uvjustin commented Nov 8, 2022

frenck Nov 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akx Nov 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akx Nov 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akx commented Nov 8, 2022 •

edited by epenet

epenet Nov 8, 2022 •

edited

frenck Nov 9, 2022 •

edited

akx Nov 9, 2022 •

edited

akx Nov 9, 2022 •

edited