Variant Plugin Auto install #86

DEKHTIARJonathan · 2025-05-07T04:48:31Z

Alright - this PR is fairly large so here are a few commands you can run to test:

Must-Do / Prepare

pip install --index-url=https://mockhouse.wheelnext.dev/pep-xxx-variants/ provider-fictional-hw

1. Make-Variant

$ variantlib make-variant -p provider_fictional_hw.plugin:FictionalHWPlugin -P "fictional_hw :: architecture :: deepthought" -f dummy_project-1.0.0-py2.py3-none-any.whl -o .
variantlib.plugins.py_envs - INFO - Using externally managed python environment
variantlib.plugins.loader - INFO - Loading plugin via provider_fictional_hw.plugin:FictionalHWPlugin
variantlib.commands.make_variant - INFO - Variant Wheel Created: `/workspace/dummy-projects-flit/dummy-project/dist/dummy_project-1.0.0-py2.py3-none-any-ed8d6c4c.whl`

2. Analyze Platform

$ variantlib analyze-platform -p provider_fictional_hw.plugin:FictionalHWPlugin
variantlib.plugins.py_envs - INFO - Using externally managed python environment
variantlib.plugins.loader - INFO - Loading plugin via provider_fictional_hw.plugin:FictionalHWPlugin
variantlib.commands.analyze_platform - INFO - Analyzing the platform ...


#################### Provider Config: `fictional_hw` ####################
        - Variant Config [001]: architecture :: ['deepthought', 'hal9000']
        - Variant Config [002]: compute_capability :: ['10', '6', '2']
        - Variant Config [003]: humor :: ['0', '2']
        - Variant Config [004]: compute_accuracy :: ['1000', '0', '10']
#########################################################################

3. Config Setup

$ variantlib config setup -p provider_fictional_hw.plugin:FictionalHWPlugin
variantlib.plugins.py_envs - INFO - Using externally managed python environment
variantlib.plugins.loader - INFO - Loading plugin via provider_fictional_hw.plugin:FictionalHWPlugin
Final configuration:

property_priorities = []
feature_priorities = []
namespace_priorities = ["fictional_hw"]

4. Plugin Commands

$ variantlib plugins -p provider_fictional_hw.plugin:FictionalHWPlugin list
variantlib.plugins.py_envs - INFO - Using externally managed python environment
variantlib.plugins.loader - INFO - Loading plugin via provider_fictional_hw.plugin:FictionalHWPlugin
fictional_hw

$ variantlib plugins -p provider_fictional_hw.plugin:FictionalHWPlugin get-all-configs
variantlib.plugins.py_envs - INFO - Using externally managed python environment
variantlib.plugins.loader - INFO - Loading plugin via provider_fictional_hw.plugin:FictionalHWPlugin
fictional_hw :: architecture :: deepthought
fictional_hw :: architecture :: hal9000
...
fictional_hw :: humor :: 6
fictional_hw :: humor :: 8

$ variantlib plugins -p provider_fictional_hw.plugin:FictionalHWPlugin get-supported-configs
variantlib.plugins.py_envs - INFO - Using externally managed python environment
variantlib.plugins.loader - INFO - Loading plugin via provider_fictional_hw.plugin:FictionalHWPlugin
fictional_hw :: architecture :: deepthought
fictional_hw :: architecture :: hal9000
...
fictional_hw :: compute_accuracy :: 0
fictional_hw :: compute_accuracy :: 10

Demo Install from `pip`

Preparation of the environment

python -m venv tmp_venv
source tmp_venv/bin/activate
pip install -e variantlib
pip install -e pip
pip config set --site global.index-url https://mockhouse.wheelnext.dev/pep-xxx-variants/

Now the install

$ pip install --dry-run dummy-project

Looking in indexes: https://mockhouse.wheelnext.dev/pep-xxx-variants/
  Creating isolated environment: uv ...
  Installing packages in current environment:
  - provider-fictional-hw == 1.0.0
  - provider-fictional-tech == 1.0.0
  Loading plugin via provider_fictional_hw.plugin:FictionalHWPlugin
  Loading plugin via provider_fictional_tech.plugin:FictionalTechPlugin
  Variant `03e04d5e` has been rejected because one or many of the variant properties `[fictional_hw :: architecture :: mother, fictional_hw :: compute_capability :: 4]` are not supported or have been explicitly rejected.
  Variant `808c7f9d` has been rejected because one or many of the variant properties `[fictional_tech :: risk_exposure :: 1000000000, fictional_tech :: technology :: improb_drive]` are not supported or have been explicitly rejected.
  Variant `80fa16ff` has been rejected because one or many of the variant properties `[fictional_hw :: architecture :: tars, fictional_hw :: compute_accuracy :: 8, fictional_hw :: compute_capability :: 8, fictional_hw :: humor :: 10]` are not supported or have been explicitly rejected.
  Total Number of Compatible Variants: 4
Collecting dummy-project
  Downloading https://mockhouse.wheelnext.dev/pep-xxx-variants/dummy-project/dummy_project-1.0.0-py2.py3-none-any-36028aca.whl (1.3 kB)
Would install dummy-project-1.0.0-36028aca

mgorny · 2025-05-07T12:53:27Z

variantlib/api.py

-        itertools.chain.from_iterable(
-            provider_cfg.to_list_of_properties()
-            for provider_cfg in plugin_loader.get_supported_configs().values()
+    with PluginLoader(variant_nfo=parsed_variants_json, isolated=False) as plugin_ctx:


I'm confused at what's happening here. What's the plan for isolated=True support?

mgorny · 2025-05-07T12:54:56Z

variantlib/installer.py

+def _get_pip_index_urls() -> tuple[list[str], list[str]]:
+    # Load pip configuration
+    configuration = pip._internal.configuration.Configuration(  # noqa: SLF001
+        isolated=False, load_only=None
+    )
+    configuration.load()
+
+    def unpack_pip_index_config_value(val: str | list[str] | tuple[str]) -> list[str]:
+        val: list[str] | tuple[str]
+        return [v.strip() for v in val.split("\n") if v.strip()]
+
+    # Retrieve index-url and extra-index-url values
+    # index_url = configuration.get_value("global.index-url")
+    # extra_index_urls = configuration.get_value("global.extra-index-url")
+    index_url = []
+    extra_index_urls = []
+    for key, val in configuration.items():
+        if key in ["global.index-url", "install.index-url"]:
+            index_url.extend(unpack_pip_index_config_value(val))
+        if key in ["global.extra-index-url", "install.extra-index-url"]:
+            extra_index_urls.extend(unpack_pip_index_config_value(val))
+
+    return list(set(index_url)), list(set(extra_index_urls))


This looks out of place. I don't think it's really for variantlib to deal with pip configs.

mgorny · 2025-05-07T12:56:13Z

variantlib/installer.py

+    _env_backend: _EnvBackend
+
+    def __init__(self) -> None:
+        if shutil.which("uv") is not None:


This is a risky assumption. Given that the code will be primarily used by pip, I dare say it should be using pip rather than calling uv internally.

mgorny · 2025-05-07T12:56:54Z

variantlib/installer.py

+    def install_requirements(
+        self, requirements: Collection[str], py_exec: str | None = None
+    ) -> None:
+        index_urls, extra_index_urls = _get_pip_index_urls()


Do we actually need to handle pip config for pip?

mgorny · 2025-05-07T12:58:11Z

variantlib/installer.py

+    ) -> None:
+        index_urls, extra_index_urls = _get_pip_index_urls()
+
+        with self.prepare_requirements(requirements) as req_file:


Do we need to worry that pip will call pip, which in turn will call pip and we'd end up with infinite recursion over this?

mgorny · 2025-05-07T12:58:44Z

variantlib/installer.py

+
+    def __enter__(self) -> Self:
+        try:
+            self._path = pathlib.Path(tempfile.mkdtemp(prefix="variant-env-"))


tempfile.TemporaryDirectory?

mgorny · 2025-05-07T12:59:21Z

variantlib/installer.py

+
+    def _create_venv(self, path: pathlib.Path) -> tuple[str, str]:
+        try:
+            import virtualenv


We shouldn't be using virtualenv at all. venv is supported on all Python versions which we support, it's simpler and faster.

mgorny · 2025-05-07T13:00:10Z

variantlib/installer.py

+            try:
+                venv.EnvBuilder(
+                    symlinks=_fs_supports_symlink(),
+                    with_pip=True,


Shouldn't this be false?

mgorny · 2025-05-07T13:01:12Z

variantlib/loader.py

+        if self._installer_ctx is None or self._plugins is not None:
+            raise RuntimeError(
+                "Impossible to get supported configs outside of an installer context"
+            )


Honestly, it looks like instead of using two separate classes for inside and outside the context, you are trying to use a single class which doesn't really seem to have any benefit.

mgorny · 2025-05-07T13:01:55Z

variantlib/loader.py

+                    location=pathlib.Path(
+                        self._installer_ctx.python_executable
+                    ).parent.parent,


This doesn't seem like a robust assumption to make.

By the way, does this logic handle dependencies? How does it handle different versions of modules present in the isolated and host environments (say, variantlib)?

charliermarsh · 2025-05-07T23:36:42Z

Hmm -- I was assuming that the installer (i.e., the thing calling variantlib) would be responsible for all of the plugin interaction, and that variantlib would just consist of pure functions (similar to packaging). The fact that pip would call variantlib, which would then call pip, seems like a sign that the control flow is going the wrong way, maybe?

Copying over my comments from Discord:

I'll need to look at the design more closely, but I think it's unlikely that you'll want variantlib to be responsible for installing and managing an environment
I was imaging that variantlib was like packaging: a well-isolated library, small enough to be vendored, that implements the standard
I don't think it should rely on an installer. It's also really hard to respect user settings, etc., if you're calling uv (for example) from within variantlib.
I guess I was assuming that the installer would interact with the plugins, and the variantlib API would be simple enough that you're just passing in data and getting data out without having to interact with any external systems?
So the installer would install the plugin in an isolated environment, ask it for the enabled variants, then pass those to variantlib, etc.

mgorny · 2025-05-08T03:47:37Z

@charliermarsh, that's pretty much what I said — we need to hook into installer's isolation and install logic, which is already there. I'm currently working on pip, so hopefully I'll get there and find a reasonably clean way of doing that.

mgorny · 2025-05-08T04:48:13Z

variantlib/api.py

@@ -50,22 +54,34 @@
 def get_variant_hashes_by_priority(
    *,
    variants_json: dict,
-    plugin_loader: PluginLoader,


I don't really like losing the flexibility of being able to provide a custom PluginLoader instance and effectively forcing people to copy over the whole logic if they need to do something nonstandard. At least until it's 100% clear that we won't need it ever.

mgorny · 2025-05-08T04:48:46Z

variantlib/api.py

@@ -50,22 +54,34 @@
 def get_variant_hashes_by_priority(
    *,
    variants_json: dict,
-    plugin_loader: PluginLoader,
+    use_auto_install: bool = True,
+    venv_path: str | pathlib.Path | None = None,


Why not just require Path? It's a new API.

I think it's pretty rare for APIs to absolutely force a Path.
In general people have a silent conversation inside APIs

variantlib/plugins/py_envs.py

mgorny · 2025-05-08T04:53:28Z

variantlib/plugins/py_envs.py

+                yield ctx
+
+    except Exception:
+        logger.exception("An error occured during plugin installation.")


Do we really want to silently proceed when environment setup fails? Wouldn't this lead to unpredictable behavior and/or confusing failures afterwards?

I wasn't sure ... I can raise an exception if you want. Would you create a new custom one ?

mgorny · 2025-05-08T04:56:22Z

variantlib/plugins/py_envs.py

+                sys.path.insert(0, str(ctx.package_dir))
+                yield ctx
+                sys.path = original_sys_path


I've already said and I'll repeat it: switching context to a different environment without process isolation is a very bad idea. If anything, you end up mixing dependencies from inside the isolated environment with already-loaded modules from the environment running variantlib. Effectively, plugin dependencies won't be respected and plugins will end up randomly using different versions of their dependencies than they specified (and we've installed).

Fair I agree - so how would you fix that ? Because it's not like variantlib can access a different venv.
I thought about this issue - but I didnt have a better idea

Honestly, let's leave it for now and "back to the drawing board" after PyCon. I'm afraid we need to change how plugins work.

mgorny · 2025-05-08T05:00:18Z

variantlib/plugins/py_envs.py

+class IsolatedPythonEnvMixin:
+    _venv_path: pathlib.Path | None = None
+
+    @property
+    def python_executable(self) -> pathlib.Path:
+        return self._get_venv_path(0)
+
+    @property
+    def script_dir(self) -> pathlib.Path:
+        return self._get_venv_path(1)
+
+    @property
+    def package_dir(self) -> pathlib.Path:
+        return self._get_venv_path(2)
+
+    def _get_venv_path(self, idx: int) -> pathlib.Path:
+        if self._venv_path is None or not self._venv_path.exists():
+            raise FileNotFoundError
+        assert 0 <= idx <= 2
+        return pathlib.Path(_find_executable_and_scripts(str(self._venv_path))[idx])


I don't get this at all. Why aren't we just calling _find_executable_and_scripts() in __init__() and setting them all as class attributes?

Oh because the venv is not created when the environment is created but rather when you enter the contextmanager.

Maybe this could be done differently - feel free to experiment

Refactoring in Progress

9c0ffcf

DEKHTIARJonathan requested a review from mgorny May 7, 2025 04:48

DEKHTIARJonathan changed the base branch from main to dev May 7, 2025 04:48

DEKHTIARJonathan marked this pull request as draft May 7, 2025 04:48

mgorny reviewed May 7, 2025

View reviewed changes

mgorny mentioned this pull request May 7, 2025

New API bits for pip: plugin loading, and wheel file supported check #87

Merged

resync

375d04a

mgorny reviewed May 8, 2025

View reviewed changes

Unittests working

b5322b7

DEKHTIARJonathan marked this pull request as ready for review May 8, 2025 05:08

Adding ManualPluginLoader

ddee54b

DEKHTIARJonathan force-pushed the auto_install branch from 757a502 to ddee54b Compare May 8, 2025 06:39

Switching to non-isolated by default

0ae602c

DEKHTIARJonathan changed the title ~~[WIP] Variant Plugin Auto install~~ Variant Plugin Auto install May 8, 2025

DEKHTIARJonathan merged commit d41967c into dev May 8, 2025
44 checks passed

DEKHTIARJonathan deleted the auto_install branch May 8, 2025 14:26

Variant Plugin Auto install #86

Variant Plugin Auto install #86

Uh oh!

Conversation

DEKHTIARJonathan commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Must-Do / Prepare

1. Make-Variant

2. Analyze Platform

3. Config Setup

4. Plugin Commands

Demo Install from pip

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charliermarsh commented May 7, 2025

Uh oh!

mgorny commented May 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DEKHTIARJonathan commented May 7, 2025 •

edited

Loading

Demo Install from `pip`