Skip to content

Improve responsiveness when invoked via Python#9315

Merged
charliermarsh merged 2 commits intoastral-sh:mainfrom
ofek:patch-1
Dec 31, 2023
Merged

Improve responsiveness when invoked via Python#9315
charliermarsh merged 2 commits intoastral-sh:mainfrom
ofek:patch-1

Conversation

@ofek
Copy link
Copy Markdown
Contributor

@ofek ofek commented Dec 29, 2023

This saves a handful of milliseconds on Windows and even more on other platforms when running python -m ruff. On non-Windows systems the process is replaced directly (impossible on Windows unfortunately).

❯ docker run --rm python:3.11 bash -c "for i in {1..15}; do python -m timeit -n 1 -r 1 'from pathlib import Path'; done"
1 loop, best of 1: 25.7 msec per loop
1 loop, best of 1: 3.07 msec per loop
1 loop, best of 1: 3.16 msec per loop
1 loop, best of 1: 3.06 msec per loop
1 loop, best of 1: 3.32 msec per loop
1 loop, best of 1: 3.93 msec per loop
1 loop, best of 1: 3.26 msec per loop
1 loop, best of 1: 3.73 msec per loop
1 loop, best of 1: 3.1 msec per loop
1 loop, best of 1: 3.29 msec per loop
1 loop, best of 1: 3.12 msec per loop
1 loop, best of 1: 3.05 msec per loop
1 loop, best of 1: 3.04 msec per loop
1 loop, best of 1: 3.19 msec per loop
1 loop, best of 1: 3.04 msec per loop

❯ docker run --rm python:3.11 bash -c "for i in {1..15}; do python -m timeit -n 1 -r 1 'import subprocess'; done"
1 loop, best of 1: 31.2 msec per loop
1 loop, best of 1: 3.75 msec per loop
1 loop, best of 1: 4.71 msec per loop
1 loop, best of 1: 3.88 msec per loop
1 loop, best of 1: 4.08 msec per loop
1 loop, best of 1: 4.35 msec per loop
1 loop, best of 1: 3.94 msec per loop
1 loop, best of 1: 4.06 msec per loop
1 loop, best of 1: 3.88 msec per loop
1 loop, best of 1: 3.85 msec per loop
1 loop, best of 1: 3.84 msec per loop
1 loop, best of 1: 4.01 msec per loop
1 loop, best of 1: 4.21 msec per loop
1 loop, best of 1: 4.07 msec per loop
1 loop, best of 1: 4.11 msec per loop

❯ python -m timeit -n 1 -r 1 "from pathlib import Path"
1 loop, best of 1: 5.25 msec per loop

❯ python -m timeit -n 1 -r 1 "import subprocess"
1 loop, best of 1: 7.61 msec per loop

Comment thread python/ruff/__main__.py
@zanieb
Copy link
Copy Markdown
Member

zanieb commented Dec 29, 2023

Hey :)

Can you show the performance changes for invoking Ruff itself rather than microbenchmarks of the import? It's hard to tell if these changes are worthwhile in our use-case. I generally prefer the readability of undeferred imports and use of Path when it's not critical.

@ofek
Copy link
Copy Markdown
Contributor Author

ofek commented Dec 29, 2023

❯ hyperfine -m 10 --warmup 1 "python -m ruff --help"
Benchmark 1: python -m ruff --help
  Time (mean ± σ):      78.3 ms ±   1.8 ms    [User: 0.0 ms, System: 0.0 ms]
  Range (min … max):    75.7 ms …  83.3 ms    34 runs

❯ hyperfine -m 10 --warmup 1 "python -m ruff --help"
Benchmark 1: python -m ruff --help
  Time (mean ± σ):      73.4 ms ±   1.0 ms    [User: 0.0 ms, System: 1.2 ms]
  Range (min … max):    71.7 ms …  75.7 ms    36 runs

I can't easily test right now in Docker, these are on my Windows machine.

@github-actions
Copy link
Copy Markdown
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Copy link
Copy Markdown
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.

@charliermarsh charliermarsh added the cli Related to the command-line interface label Dec 29, 2023
@charliermarsh charliermarsh merged commit 158367b into astral-sh:main Dec 31, 2023
@ofek ofek deleted the patch-1 branch December 31, 2023 16:36
@zanieb zanieb mentioned this pull request Feb 19, 2024
meta-codesync bot pushed a commit to facebook/dotslash that referenced this pull request Oct 31, 2025
Summary:
Hello again! This is the second [half](#82) of what we require in order to start using DotSlash.

This moves over the [code](https://github.com/ofek/dotslash-python) for the `dotslash` Python [package](https://pypi.org/project/dotslash/) I created over the weekend. The ownership transfer on PyPI is in progress.

The package serves a similar purpose to the existing `fb-dotslash` Node.js [package](https://www.npmjs.com/package/fb-dotslash) stored in the top level [`node`](https://github.com/facebook/dotslash/tree/main/node) directory. Some notes:

- Python 3.9 is EOL at the end of the month so I chose to require 3.10+.
- The package exposes a single `locate` function for finding the path to the binary that was installed by the installation. The logic was adapted from equivalents like UV's [finder](https://github.com/astral-sh/uv/blob/141369ce73b7b0b4e005b0f45107d13c828a99e0/python/uv/_find_uv.py). I optimized the path resolution to perform as little work as necessary and to do so lazily.
    ```
    ❯ docker run --rm -it python:3.14 bash
    root@8a442038a1c8:/# curl -fsSL https://github.com/sharkdp/hyperfine/releases/download/v1.19.0/hyperfine-v1.19.0-x86_64-unknown-linux-musl.tar.gz \
    | tar -xzO 'hyperfine-v1.19.0-x86_64-unknown-linux-musl/hyperfine' \
    | install -m 0755 /dev/stdin /usr/local/bin/hyperfine
    root@8a442038a1c8:/# pip install -qqq dotslash uv
    root@8a442038a1c8:/# hyperfine -m 100 --warmup 10 "python -m timeit -n 1 -r 1 -s 'from uv import find_uv_bin' 'find_uv_bin()'"
    Benchmark 1: python -m timeit -n 1 -r 1 -s 'from uv import find_uv_bin' 'find_uv_bin()'
      Time (mean ± σ):      33.6 ms ±   2.4 ms    [User: 25.3 ms, System: 7.6 ms]
      Range (min … max):    30.3 ms …  46.1 ms    100 runs

    root@8a442038a1c8:/# hyperfine -m 100 --warmup 10 "python -m timeit -n 1 -r 1 -s 'from dotslash import locate' 'locate()'"
    Benchmark 1: python -m timeit -n 1 -r 1 -s 'from dotslash import locate' 'locate()'
      Time (mean ± σ):      23.5 ms ±   1.0 ms    [User: 17.8 ms, System: 5.7 ms]
      Range (min … max):    22.1 ms …  26.9 ms    127 runs

    root@8a442038a1c8:/# # Show time it takes to locate binaries
    root@8a442038a1c8:/# python -m timeit -n 1 -r 1 -s "from uv import find_uv_bin" "find_uv_bin()"
    1 loop, best of 1: 7.71 msec per loop
    root@8a442038a1c8:/# # This includes the time of all imports (still faster)
    root@8a442038a1c8:/# python -m timeit -n 1 -r 1 -s "from dotslash import locate" "locate()"
    1 loop, best of 1: 2.86 msec per loop
    root@8a442038a1c8:/# # This excludes import times to match eager behavior of UV
    root@8a442038a1c8:/# python -m timeit -n 1 -r 1 -s "from dotslash import locate,_locate" "locate()"
    1 loop, best of 1: 1.14 msec per loop
    ```
- The shipped binary can be invoked directly with `python -m dotslash`. If there ever is a desire to represent paths as anything other than strings in the implementation, please [don't](astral-sh/ruff#9315).
- Builds require setting the `DOTSLASH_VERSION` environment variable to the desired release tag of DotSlash (the `v` is optional). This will be used as the package version and, by default, for fetching the appropriate release artifact.
- Rather than making HTTP requests during builds, you can set the `DOTSLASH_SOURCE` environment variable to the path to a directory containing the release artifacts. I strongly recommend that the release process uses that eventually (and the Node.js package's [build script](https://github.com/facebook/dotslash/blob/646102b468bb5e820e45d3d06a7ee63e13b51d66/node/scripts/build-package.js) gains support).
- I remove build instructions from the content of the PyPI project's landing page since the information wouldn't be useful for users.
- I moved the release jobs of the downstream packages into a separate workflow that only executes when called by the primary release workflow. I already configured Trusted Publishing on PyPI to use the `release-downstream.yml` workflow from this repository. You might find hazy details about their support for reusable workflows but [this comment](pypi/warehouse#11096 (comment)) provides the full picture. Basically, the claim they chose happens to work for the most nested called workflow when using reusable workflows but not the top level one. Whenever support for that happens there will be no user impact for existing supported scenarios.

 ---

cc sdwilsh bigfootjon for review as requested

Also, it's worth noting that PyPI was blocking the name `dotslash` due to certain [rules](https://github.com/pypi/warehouse/blob/main/warehouse/migrations/versions/d18d443f89f0_ultranormalize_name_function.py) and di very graciously provided the override 🙇‍♂️

Pull Request resolved: #87

Test Plan: CI

Reviewed By: bigfootjon

Differential Revision: D84891338

Pulled By: sdwilsh

fbshipit-source-id: 493086f3a0106d0c0e83c88eeb82bfe9ba277507
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli Related to the command-line interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants