Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default to PEP 517-based builds #843

Merged
merged 3 commits into from
Jan 10, 2024
Merged

Default to PEP 517-based builds #843

merged 3 commits into from
Jan 10, 2024

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Jan 9, 2024

Summary

Our current setup uses the legacy setup.py-based builds if a pyproject.toml file isn't present. This matches pip's behavior. However, pypa/build uses PEP 517-based builds in such cases, and it looks like pip plans to make that the default (pypa/pip#9175), with the limiting factor being performance issues related to isolated builds.

This is now the default behavior, but the --legacy-setup-py flag allows users to opt-in to using setup.py directly for distributions that lack a pyproject.toml.

@charliermarsh
Copy link
Member Author

So it looks like resolution is meaningfully faster:

Benchmark 1: ./target/release/main (resolve-cold)
  Time (mean ± σ):     566.4 ms ±  34.8 ms    [User: 213.1 ms, System: 613.3 ms]
  Range (min … max):   528.0 ms … 715.2 ms    50 runs

Benchmark 2: ./target/release/puffin (resolve-cold)
  Time (mean ± σ):     518.0 ms ±  31.1 ms    [User: 217.3 ms, System: 293.8 ms]
  Range (min … max):   482.7 ms … 630.0 ms    50 runs

But installation is meaningfully slower:

Benchmark 1: ./target/release/main (install-cold)
  Time (mean ± σ):     683.2 ms ±  23.5 ms    [User: 276.4 ms, System: 678.9 ms]
  Range (min … max):   646.0 ms … 765.9 ms    50 runs

Benchmark 2: ./target/release/puffin (install-cold)
  Time (mean ± σ):     712.1 ms ±  28.2 ms    [User: 328.3 ms, System: 358.7 ms]
  Range (min … max):   673.3 ms … 814.9 ms    50 runs

@charliermarsh
Copy link
Member Author

This using django-ipware==3.0.0 somewhat arbitrarily, as a simple package that uses legacy setup.py.

@charliermarsh
Copy link
Member Author

Okay, skipping get_requires_for_build_wheel in this case makes both much faster:

Benchmark 1: ./target/release/main (install-cold)
  Time (mean ± σ):     692.8 ms ±  52.3 ms    [User: 280.0 ms, System: 644.6 ms]
  Range (min … max):   633.7 ms … 970.7 ms    40 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 2: ./target/release/puffin (install-cold)
  Time (mean ± σ):     604.4 ms ±  29.5 ms    [User: 247.7 ms, System: 337.3 ms]
  Range (min … max):   555.3 ms … 677.1 ms    40 runs

Summary
  './target/release/puffin (install-cold)' ran
    1.15 ± 0.10 times faster than './target/release/main (install-cold)'
Benchmark 1: ./target/release/main (resolve-cold)
  Time (mean ± σ):     589.5 ms ±  72.2 ms    [User: 215.3 ms, System: 610.5 ms]
  Range (min … max):   525.1 ms … 870.2 ms    40 runs

Benchmark 2: ./target/release/puffin (resolve-cold)
  Time (mean ± σ):     461.1 ms ±  35.0 ms    [User: 164.1 ms, System: 275.6 ms]
  Range (min … max):   426.1 ms … 564.6 ms    40 runs

I think that's fine to do, honestly?

@charliermarsh charliermarsh requested a review from konstin January 9, 2024 02:19
@charliermarsh charliermarsh marked this pull request as ready for review January 9, 2024 02:20
@charliermarsh charliermarsh force-pushed the charlie/legacy-setup branch 2 times, most recently from 6819601 to 1aa97b0 Compare January 9, 2024 02:30
Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love it if we could get away with this!

The perf improvement is impressive!

@charliermarsh
Copy link
Member Author

@konstin - Two questions: (1) should we add a flag to disable this? I kind of think we should. And (2) why do you think cold-install got faster?

@konstin
Copy link
Member

konstin commented Jan 9, 2024

should we add a flag to disable this? I kind of think we should.

Agreed

why do you think cold-install got faster?

Good question, should i look into it?

@charliermarsh
Copy link
Member Author

Sounds good, I'll add the flag.

Yeah, if you can, that would be great. I was comparing main to this branch via bench script on django-ipware==3.0.0.


// If we're using the default backend configuration, skip `get_requires_for_build_*`, since
// we already installed the requirements above.
if pep517_backend != default_backend {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized... we can actually skip this if we know the build backend doesn't implement it? Like poetry-core always returns [].

@charliermarsh charliermarsh force-pushed the charlie/prepare-metadata branch 5 times, most recently from f16ec58 to c9c2e08 Compare January 10, 2024 00:03
Base automatically changed from charlie/prepare-metadata to main January 10, 2024 00:07
@charliermarsh charliermarsh force-pushed the charlie/legacy-setup branch 3 times, most recently from 9102ee4 to 0a211cb Compare January 10, 2024 01:19
@charliermarsh charliermarsh added the enhancement New feature or improvement to existing functionality label Jan 10, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) January 10, 2024 01:20
@charliermarsh charliermarsh merged commit 55f2be7 into main Jan 10, 2024
@charliermarsh charliermarsh deleted the charlie/legacy-setup branch January 10, 2024 01:27
@konstin
Copy link
Member

konstin commented Jan 11, 2024

What requirements and python version did you use for the benchmarking numbers? I get no difference when building django-ipware-3.0.0.tar.gz by hand:

$ hyperfine --warmup 1 --prepare "rm -rf a dist" "python setup.py bdist_wheel" "python -c \"from setuptools.build_meta import __legacy__; __legacy__.build_wheel('a')\""
Benchmark 1: python setup.py bdist_wheel
  Time (mean ± σ):      88.8 ms ±   1.1 ms    [User: 79.8 ms, System: 9.2 ms]
  Range (min … max):    87.0 ms …  92.5 ms    32 runs
 
Benchmark 2: python -c "from setuptools.build_meta import __legacy__; __legacy__.build_wheel('a')"
  Time (mean ± σ):      89.8 ms ±   2.1 ms    [User: 80.6 ms, System: 9.1 ms]
  Range (min … max):    87.7 ms …  97.6 ms    32 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs.
 
Summary
  python setup.py bdist_wheel ran
    1.01 ± 0.03 times faster than python -c "from setuptools.build_meta import __legacy__; __legacy__.build_wheel('a')"

@charliermarsh
Copy link
Member Author

I suspect that there is no difference when building by hand (there shouldn't be, right? the PEP 517 interface is just a thin wrapper). Instead I suspect the difference is from something we're doing in puffin-build.

charliermarsh added a commit that referenced this pull request Jun 24, 2024
## Summary
PEP 517 build isolation #843 has not
yet been mentioned in the PIP compatibility documentation. Add a section
for it.

## Test Plan
Visual inspection only

## Open Questions
> in most cases, swapping out `pip install` for `uv pip install` should
"just work".

In the first non-trivial case I tried, it worked for a short time and
then [started
failing](#4069 (comment)).
Is there any data out there on how many top 100 or top 1000 packages
work with PEP 517 build isolation?

How can someone specify `--no-build-isolation` for just one package/line
in `requirements.txt`?

---------

Co-authored-by: Charlie Marsh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants