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

Use uv run to run the Python-based fuzzer #13074

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

AlexWaygood
Copy link
Member

Summary

This converts the scripts/fuzz-parser script into a script runnable via uv run.

Since this is a single-file script, I considered using inline metadata here; it seemed like it might be a good fit. However, it seems as though inline dependencies aren't recorded in uv.lock at all, which wouldn't be ideal since we use this script in CI as a regression test. I therefore added the dependencies of the script to the pyproject.toml in the Ruff repo root instead, which means that the dependencies are pinned in the project's uv.lock file.

Annoyingly, uv run scripts/fuzz-parser.py seems to build Ruff from source with this PR before it runs the script. I don't think there's any way currently to tell uv to only install the dev dependencies, and not bother with installing the project itself, when running scripts? Building Ruff from source isn't necessary with this script if you pass the --test-executable flag. This might be a blocker for this PR? It means that the uv run invocation takes ages to get going.

I deliberately haven't tried to include any of the docs dependencies in this change, as @MichaReiser already tried in #12622, and the situation there is quite complicated with the two requirements.txt files.

Overall this is something I'd love to do as it seems like it could be a really nice simplification... but I'm not sure it's a perfect fit right now?

Test Plan

  • uv run scripts/fuzz-parser.py 0-100
  • uv run scripts/generate_known_standard_library.py
  • cd scripts/knot_benchmark && uv run benchmark

@AlexWaygood AlexWaygood added internal An internal refactor or improvement ci Related to internal CI tooling labels Aug 23, 2024

This comment was marked as spam.

Copy link
Contributor

github-actions bot commented Aug 23, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

warning: Detected debug build without --no-cache.
error: Failed to parse examples/chatgpt/gpt_actions_library/gpt_action_salesforce.ipynb:14:1:1: Expected an expression

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to parse examples/chatgpt/gpt_actions_library/gpt_action_salesforce.ipynb:14:1:1: Expected an expression

@MichaReiser
Copy link
Member

Haha, and here is another use case asking for virtual workspace. This will make @charliermarsh so happy

@MichaReiser MichaReiser requested review from charliermarsh and removed request for MichaReiser August 23, 2024 11:14
@dhruvmanila dhruvmanila removed their request for review August 23, 2024 11:40
@AlexWaygood
Copy link
Member Author

Well... this works for the fuzz-parser script, but not for generate_known_standard_library.py. uv refuses to install a new enough version of stdlibs, because recent versions of stdlibs require Python 3.8+, but the requires-python field in our pyproject.toml states (correctly) that Ruff supports being installed on Python 3.7+, and it seems impossible to convince uv that it's okay to install a dev-dependency that's only available on newer Python versions if there's the requires-python = ">= 3.7" field there.

I think having a dev-dependency that's only available on newer Python versions than the one the package is declared as supporting is probably reasonable...? Hmm, but maybe if it's declared as a "virtual" workspace, uv assumes there is no package, so therefore the requires-python field must refer to whatever ad-hoc scripts you have floating around in your "virtual" workspace...

@charliermarsh
Copy link
Member

I kind of think these should just be PEP 723 scripts. Semantically, at least, that's what they want to be, right?

@AlexWaygood
Copy link
Member Author

I kind of think these should just be PEP 723 scripts. Semantically, at least, that's what they want to be, right?

I tried that at first, but I don't get the transitive dependencies pinned in the lockfile (and updated by Renovate) if I have them as PEP 723 scripts, right? We run fuzz-parser in CI; I think we want the transitive deps pinned for that

@charliermarsh
Copy link
Member

Separately, why are the deps not in scripts/pyproject.toml?

@AlexWaygood
Copy link
Member Author

Separately, why are the deps not in scripts/pyproject.toml?

Would I have to cd into scripts to run them then? fuzz-parser needs to be run from the workspace root. Or will uv run pick up the config file at scripts/pyproject.toml even if I run the scripts from the workspace root?

@zanieb
Copy link
Member

zanieb commented Aug 28, 2024

Related astral-sh/uv#6733

@charliermarsh
Copy link
Member

Alternatively we could prioritize adding the lockfile to PEP 723 scripts heh.

@AlexWaygood
Copy link
Member Author

Alternatively we could prioritize adding the lockfile to PEP 723 scripts heh.

Ah, that would be the ideal solution if it's something you're considering!

@zanieb
Copy link
Member

zanieb commented Aug 29, 2024

@AlexWaygood would you want it to be embedded in the script? It might be looong.

@AlexWaygood
Copy link
Member Author

@AlexWaygood would you want it to be embedded in the script? It might be looong.

Hrm... no, probably not!

I think there's essentially two genres of single-file scripts:

  1. One-off scripts that you write for a single purpose. You want these to be easily shareable, you don't want to have to faff around with a separate requirements.txt or lockfile for them, you want everything about the script to be contained within the single file.
  2. "Persistent" scripts that are a single file but that you keep around indefinitely and run on a regular basis. These might be scripts that are e.g. run as part of CI for a larger project. You want these scripts to be reliable, and shareability isn't as important.

These scripts definitely feel like they fit into category (2) rather than category (1). It's not like it's ever going to make sense to try to run them outside this repository, so shareability and portability aren't really important; it's okay if the lockfile gets output to a separate file, IMO. I guess the easiest way of doing that implementation-wise would be a separate lockfile for each script, since you'll probably want a separate virtual environment for each script?

@charliermarsh
Copy link
Member

"Persistent" scripts that are a single file but that you keep around indefinitely and run on a regular basis. These might be scripts that are e.g. run as part of CI for a larger project. You want these scripts to be reliable, and shareability isn't as important.

I think this should just be a project, personally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Related to internal CI tooling internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants