core icon indicating copy to clipboard operation
core copied to clipboard

Make pip_kwargs consistent with async_mount_local_lib_path

Open ruifung opened this issue 11 months ago • 5 comments

Breaking change

Proposed change

The existing requirements.pip_kwargs function, seems like it intended, under specific circumstances (not virtualenv and not docker, so CORE installations?), to install additional dependencies into the directory mounted by bootstrap.async_mount_local_lib_path (/config/deps/..).

However, what actually ends up happening is that pip_kwargs sets target=/config/deps while async_mount_local_lib_path mounts /config/deps/python3.13/site-packages into sys.path.

This results in HASS installing packages into a directory it doesn't mount onto sys.path. Therefore, this PR is to correct that inconsistency by making pip_kwargs determine the target in the same way async_mount_local_lib_path does, ensuring that it will install additional dependencies into the same location it mounts.

Type of change

  • [ ] Dependency upgrade
  • [X] 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

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue: https://github.com/home-assistant/core/issues/127966
  • Link to documentation pull request:

Checklist

  • [x] The code change is tested and works locally.
  • [ ] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [ ] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • [ ] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [ ] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

ruifung avatar Jan 03 '25 10:01 ruifung

Is this still needed? Haven't read the whole comment thread on linked bug

CloCkWeRX avatar Feb 10 '25 07:02 CloCkWeRX

@CloCkWeRX under their "supported" deployment methods, you will almost never encounter any issues caused by this inconsistency.

I mostly worked around it by overwriting the run script in the container image.

ruifung avatar Feb 10 '25 07:02 ruifung

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes. Thank you for your contribution!

github-actions[bot] avatar Apr 11 '25 08:04 github-actions[bot]

Still think this is kind of a bug.

ruifung avatar Apr 13 '25 12:04 ruifung

@CloCkWeRX under their "supported" deployment methods, you will almost never encounter any issues caused by this inconsistency.

If this is not a problem for our supported installations method, why should we fix it? Please come with a real use case

I mostly worked around it by overwriting the run script in the container image.

Why are you doing that?

edenhaus avatar May 05 '25 12:05 edenhaus

Why are you doing that?

Because I want to run HASS, in a container, inside a venv, on K8S.

At the time I made this MR, HASS would, on detecting being run in a container, install dependencies to the system library path. And the issue was compounded by the linked issue where it would not correctly detect the containerized environment if it wasn't running in docker, thus running in a containerized environment, but using some code paths for CORE installations.

This is problematic for two reasons:

  1. It assumes the root filesystem is writeable.
  2. More importantly, the extra dependencies are not persisted and must be re-downloaded when the container sandbox is recreated.

This means that any deployment of the image, on an environment that does not persist the sandbox (read: K8S), will not be able to start without an internet connection.

Thus in the context of this MR, the modified start script was basically tricking HASS into properly detecting it's running in a container, which was fixed in that other issue. I'm still using a patched start script to manage instanced VENVs on a persistent mount point though, because I would prefer my HASS instance not redownload all the extra deps every time it's pod restarts.

If this is not a problem for our supported installations method, why should we fix it? Please come with a real use case

Because from what I can tell, this is a bug that was inadvertently introduced with the change to using uv for installing additional dependencies. And it's an inconsistency between where it installs extra deps, and where it mounts the extra deps under one specific scenario, as described in the MR above.

I can see this potentially still breaking if the containerized environment detection ever breaks again, and this code path is used while running in a containerized environment.

ruifung avatar May 07 '25 11:05 ruifung

Also, I feel that the whole "if it doesn't happen on a supported installation method then it's not a problem" implication here is somewhat problematic.

It's understandable if this was reporting an issue that requires effort to troubleshoot, etc,. But this is an MR, with the issue already investigated, and a fix proposed.

There are many reasons why one might run it in an unsupported installation method, such as already having a K8S environment where everything else is deployed, and wanting to ensure that it can properly restart without an internet connection. And in light of that, not wanting the overhead of running an extra VM just for home assistant.

For reference, this is my modified start script that I'm mounting into the container on my environment https://github.com/ruifung/rfhome-infrastructure/blob/master/deployments/pathweb/workloads/home-network/home-assistant/venv-run

ruifung avatar May 07 '25 11:05 ruifung

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes. Thank you for your contribution!

github-actions[bot] avatar Jul 14 '25 13:07 github-actions[bot]

Not stale, waiting for review by @MartinHjelmare

emontnemery avatar Jul 15 '25 08:07 emontnemery

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Nov 23 '25 13:11 home-assistant[bot]

Ok wow, it's been quite a while, I'll see what I can do about fixing those tests then.

ruifung avatar Nov 23 '25 23:11 ruifung

Ok wow, it's been quite a while, I'll see what I can do about fixing those tests then.

Thank you! ❤️

frenck avatar Nov 24 '25 10:11 frenck

Based on what I saw of the automated tests that failed, it seems like the python 3.14 changes removed the asyncio loop variable from init, updated code to match it.

Also rebased on to latest-ish dev.

ruifung avatar Nov 24 '25 11:11 ruifung