use poetry to manage deps for isolated build envs

[abn]

Previously, isolated build environments needed for both package installation from sdist and for fallback package metadata inspection used to rely on inline scripts and delegated management of build requirements to the build package.

With this change, Poetry manages the build requirements. This enables the use of build requirements from the current projects source, Poetry's configurations and also cache. We also, reduce the use of subprocesses and thereby increasing speeds of such builds.

Closes: #5650

[Secrus]

Some initial points

[abn]

@Secrus @radoering let me know if functionally anything needs to change. If not, I can tidy the commit up and make the PR ready once #9132 is merged - it should have an impact on the test cases.

[Secrus]

I guess it's a replacement for #5650?

[abn]

I guess it's a replacement for #5650?

Oh hey, past me had the same idea. Few things I might actually borrow from that one. Thanks for pointing that out.

Partial changes made.

[Secrus]

@abn did you maybe check the performance of this? In your previous attempt at #5650 you mentioned there was some performance regression

[abn]

It does still incur the solving penalty. But overall the test suite is faster by a little bit for the following reason.

1. We now use bare venvs. And a lot around this has improved since the old PR.
2. We remove a couple of subprocess calls.
3. From the test suite we also removed the pip freeze call that dropped 2 seconds from that test as well.

Overall I think this improves our posture. The next step for this would be to optimize cached build environments or removing the solver penalty. We can basically allow for the latest matching versions to be installed fairly confidently.

[Secrus]

The test suite is one thing, but did you check what the actual perf regression is? Is it within ~10% or more?

[Secrus]

The reason I am asking is that I would like us to know how serious we should be about improving the performace. Should that be next PR, but before next version release or can it be something down the line because the ecosystem is now in a better place and this path won't be hit that often?

[abn]

The test suite is a good indicator for any major hits.

Since we do not really have empirical data for the old build mechanism, we will have to try doing some experiments. I'll see what I can do.

Finger in the air, I'd say for majority cases (simple/common build require combinations) we would be quicker than subprocess pip install. And for the other cases, it will depend on the build requires constraints.

This change also brings with it some additional benefits that offset performance impact.

1. Cache reuse.
2. Repository configuration reuse.

[abn]

Since we do not really have empirical data for the old build mechanism, we will have to try doing some experiments. I'll see what I can do.

Did a quick hacky evaluation using the following snippet.

from pathlib import Path

from poetry.inspection.info import get_pep517_metadata

if __name__ == '__main__':
    _ = get_pep517_metadata(Path("tests/fixtures/project_with_setup"))

I ran this script 100 times and timed it.

Poetry 1.8.2

real	0m11.508s
user	0m9.374s
sys	0m2.078s

PR 9168

real    0m13.500s
user    0m11.031s
sys     0m2.388s

On average the performance hit is 0.02 seconds per run.

[radoering]

Finger in the air, I'd say for majority cases (simple/common build require combinations) we would be quicker than subprocess pip install. And for the other cases, it will depend on the build requires constraints.

Our nemesis of build requirements is probably oldest-supported-numpy. However, I suppose most packages that depend on that provide wheels. Thus, it might not be that relevant. A potential solver optimization would be to just solve for the current environment instead of solving cross-platform (only for isolated build environments) but I haven't looked into it in detail because I did not see any reports where this is a bottleneck.

In a quick real-world test, I could not observe a regression either.

[dimbleby]

@abn merging this into the "remove the setup reader" pull request causes quite a lot of test failures with stacks ending like

/home/dch/poetry/src/poetry/repositories/cached_repository.py:50: in get_release_info                                          
    return PackageInfo.load(self._get_release_info(name, version))                                                             
/home/dch/poetry/src/poetry/repositories/legacy_repository.py:114: in _get_release_info
    return self._links_to_data(                                                                                                
/home/dch/poetry/src/poetry/repositories/http_repository.py:365: in _links_to_data
    info = self._get_info_from_links(links, ignore_yanked=not data.yanked)
/home/dch/poetry/src/poetry/repositories/http_repository.py:328: in _get_info_from_links                                           
    return self._get_info_from_metadata(sdists[0]) or self._get_info_from_sdist(
/home/dch/poetry/src/poetry/repositories/http_repository.py:160: in _get_info_from_sdist
    return PackageInfo.from_sdist(filepath)
/home/dch/poetry/src/poetry/inspection/info.py:473: in from_sdist
    return cls._from_sdist_file(path=path)
/home/dch/poetry/src/poetry/inspection/info.py:329: in _from_sdist_file
    new_info = cls.from_directory(path=sdist_dir)
/home/dch/poetry/src/poetry/inspection/info.py:453: in from_directory
    info = get_pep517_metadata(path)
/home/dch/poetry/src/poetry/inspection/info.py:534: in get_pep517_metadata
    with isolated_builder(path, "wheel") as builder:
/usr/lib/python3.10/contextlib.py:135: in __enter__
    return next(self.gen)
/home/dch/poetry/src/poetry/utils/isolated_build.py:121: in isolated_builder
    pool = pool or Factory().create_poetry().pool
/home/dch/poetry/src/poetry/factory.py:57: in create_poetry
    base_poetry = super().create_poetry(cwd=cwd, with_groups=with_groups)
/home/dch/poetry-core/src/poetry/core/factory.py:49: in create_poetry
    poetry_file = self.locate(cwd)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

cls = <class 'poetry.factory.Factory'>
cwd = PosixPath('/tmp/pytest-of-dch/pytest-2/popen-gw0/test_solver_chooses_from_secon0')

    @classmethod
    def locate(cls, cwd: Path | None = None) -> Path:
        cwd = Path(cwd or Path.cwd())
        candidates = [cwd]
        candidates.extend(cwd.parents)

        for path in candidates:
            poetry_file = path / "pyproject.toml"
                return poetry_file

        else:
>           raise RuntimeError(
                f"Poetry could not find a pyproject.toml file in {cwd} or its parents"
            )
E           RuntimeError: Poetry could not find a pyproject.toml file in /tmp/pytest-of-dch/pytest-2/popen-gw0/test_solver_chooses_from_secon0 or its parents

have not yet dug into this but suspect a bug in this one?

will make similar comment in that MR for visibility, apologies if you see both!

[abn]

@dimbleby the build is expected to run within a project context. You can add something like this to the suite.

@pytest.fixture(autouse=True)
def use_project_context(set_project_context: SetProjectContext) -> Iterator[None]: 
    with set_project_context("sample_project"):
        yield

Alternatively, if the assumption that the isolated builder is only ever setup within a project context is wrong we might need to handle this in the isolated builder context.

This is the relevant line in the isolated builder.

364d16e#diff-0194c9a7c832a9f4c5679a65bd325520b6bab0c2fde3e3152c38f2aa7b64f49dR121

[dimbleby]

oh awkward. we probably ought to have been passing the poetry object along the call path all the way, this is getting a little bit spooky-action-at-a-distance. But I see that is quite far from convenient...

[abn]

Yeah, ideally it would have been nice to have the poetry instance during inspection too, but that would require some additional work I reckon.

For now, I have raised #9180 to soften the issue.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.