uv icon indicating copy to clipboard operation
uv copied to clipboard

feat: Support remote scripts with `uv run`

Open manzt opened this issue 1 year ago • 2 comments
trafficstars

First off, congratulations on the 0.3 release! The PEP 723 standalone scripts support is awesome, and I can already imagine a long tail of little scripts of my own that would benefit from this functionality.

Background

I really like the Deno CLI's support for running and installing remote scripts.

deno run <url>
deno install --name foo <url>

I can see parallels with uv run and uvx. After mentioning this on Discord, @zanieb suggested I could take a stab at a PR to implement similar functionality for uv.

Summary

This PR attempts to add support for executing remote standalone scripts directly with uv run. While this is already possible by downloading the script (i.e., via curl/wget) and then using uv run, having direct support would be convenient.

The proposed functionality is:

uv run <url>

Another addition/alternative could be to support running scripts via stdin:

curl -sL <url> | uv run -

But that is not implemented in this PR.

Test Plan

I noticed that GitHub and files.pythonhosted.org URLs are used in some of the tests. I've created a personal GitHub Gist with the example from PEP 723 for now to test this functionality.

~However, I couldn't figure out how to get the with_snapshot config filter to filter out the tempfile path, so the test is currently failing. Any assistance with this would be appreciated.~

Notes

I'm not totally pleased with the implementation of this PR. I think it would be better to handle the case earlier (and probably reuse the cache), and avoid mutation, but since run command requires a local path this was the simplest implementation I could come up with.

I know that performance is paramount with uv so I totally understand if this requires a different approach or something more explicit to avoid "inferring" the path. I'm just taking this as an opportunity to learn a little more Rust and acquaint myself with the code base. cheers!

manzt avatar Aug 21 '24 20:08 manzt

Awesome! Thanks for putting this up.

zanieb avatar Aug 21 '24 20:08 zanieb

Amazing feature. Imagine this with #6283

mgaitan avatar Aug 23 '24 18:08 mgaitan

Sorry for the delay, I've been stretched thin on reviews.

@BurntSushi would you be able to take over review of this?

zanieb avatar Sep 17 '24 04:09 zanieb

I generally have a preference for the "We could look for a file path matching the URL, and if it exists, either use the local file or report an error." option rather than prompting. Opt-in might make sense, but via a --remote flag rather than a persistent configuration option (maybe that's what you meant though).

zanieb avatar Sep 17 '24 18:09 zanieb

See also https://github.com/astral-sh/uv/issues/7396#issuecomment-2351129628

zanieb avatar Sep 17 '24 19:09 zanieb

If we go the route of prioritizing a local file when given a URL when a file exists, then I think that probably satisfies me. I can't think of a way for that to go wrong from a "accidentally executed a remote script instead of the intended local file" perspective.

There may still be an argument in favor of a --remote flag, but more in a vague "otherwise it's too easy to run remote scripts" sense. I don't feel as strongly about that personally.

@manzt What do you think?

BurntSushi avatar Sep 18 '24 13:09 BurntSushi

Thank you for raising this concern. I agree it's a valid issue I hadn't considered.

My preference aligns with @zanieb's suggestion to prioritize checking for a local file path matching the URL first, erroring if a match is found. For context, I checked your example with deno, which always makes the HTTP request (though it operates in a more sandboxed environment than Python, not sure why they decided on this behavior).

Regarding a --remote flag, while I don't have strong arguments for or against it, I appreciate its explicitness. The common use case I envision is sharing links to Gists or GitHub, where adding --remote before execution seems a reasonable safeguard (especially if the readme just has a copy link).

It's worth noting that with #6467, I believe scripts can already be executed via curl <url> | uv run - (though last time I checked the script metadata wasn't parsed from stdin). However, handling this directly in uv does seem really nice UX.

Apologies for the delayed response!

manzt avatar Sep 18 '24 17:09 manzt

All righty, let's go with checking for whether a local file exists or not.

@charliermarsh Do you have any opinions here? Do you think uv run https://example.com/foo.py should "just work," or do you think there should be an extra step needed, e.g., uv run --remote https://example.com/foo.py? (Above we discussed mitigating the case where https://example.com/foo.py actually refers to a valid file path, in which case, we would just read the local file instead.)

BurntSushi avatar Sep 19 '24 13:09 BurntSushi

I sort of prefer "just works"

zanieb avatar Sep 19 '24 14:09 zanieb

Same.

BurntSushi avatar Sep 19 '24 14:09 BurntSushi

Same. Whatever we do, it would be nice to align the behavior of requirements.txt files, which can also be either remote or local.

charliermarsh avatar Sep 19 '24 15:09 charliermarsh

Seems like agreement, I’ll start addressing the prior comments and handle the file behavior!

manzt avatar Sep 19 '24 21:09 manzt

Thanks for your patience. I think I was able to achieve the desired behavior and added another test.

I'm not sure how we should plan on testing the remote behavior (right now still depends on a gist with a fixture of my own).

manzt avatar Sep 25 '24 18:09 manzt

hmm

cargo run run https://raw.githubusercontent.com/astral-sh/uv/main/scripts/uv-run-remote-script-test.py CI

seems to be working on this branch, but the test suite is having trouble resolving the version of rich. Maybe I'm missing something in the setup of the TestContext.

manzt avatar Sep 26 '24 15:09 manzt

hmm

cargo run run https://raw.githubusercontent.com/astral-sh/uv/main/scripts/uv-run-remote-script-test.py CI

seems to be working on this branch, but the test suite is having trouble resolving the version of rich. Maybe I'm missing something in the setup of the TestContext.

Ah right my bad! Our tests set --exclude-newer to avoid new releases of packages changing resolutions. Really sorry (especially since I'm the one who asked for a version pin!), but this means we'll need another PR to change the script we just merged to use an older version of rich. I think 13.7.0 should be old enough since that was published before our EXCLUDE_NEWER value:

https://github.com/astral-sh/uv/blob/c8357b7bf23b4ade3b98be74bd7a469c05e6ec02/crates/uv/tests/common/mod.rs#L29-L30

BurntSushi avatar Sep 26 '24 15:09 BurntSushi

No worries, I opened https://github.com/astral-sh/uv/pull/7713

manzt avatar Sep 26 '24 15:09 manzt

Test is working now, pointing to #7713. Will switch to hash on main once merged.

manzt avatar Sep 26 '24 15:09 manzt

The CI failure looks legitimate. My guess is that the file path is just totally invalid on Windows, which in turn causes Path::new(...).try_exists() to fail and thus bail out of the remote logic. So I think I would change the if to this:

    // Only continue if we are absolutely certain no local file exists.
    //
    // We don't do this check on Windows since the file path would
    // be invalid anyway, and thus couldn't refer to a local file.
    if cfg!(unix) && !matches!(Path::new(target).try_exists(), Ok(false)) {
        return Ok(None);
    }

Also, I think we need to add docs to uv run mentioning this feature. :)

BurntSushi avatar Sep 27 '24 14:09 BurntSushi

Thanks for the tips!

Also, I think we need to add docs to uv run mentioning this feature. :)

I took a stab at updating the docs.

manzt avatar Sep 27 '24 15:09 manzt

I think you need to run cargo dev generate-cli-reference. Basically, changes to the docs need to be propagated out to other places.

BurntSushi avatar Sep 27 '24 15:09 BurntSushi

Ah, sorry... wasn't sure what the source of truth was. Thanks!

manzt avatar Sep 27 '24 15:09 manzt

Any thoughts on how to update the test checking for the local file? Is there a way to mark that a test should fail on windows for example?

manzt avatar Sep 27 '24 15:09 manzt

I don't think it should fail though? It looks like it's failing because it can't write a temp file? I'm not sure what the issue is there.

I think on Windows we skip checking for a local file, which would result in requestinghttps://example.com/path/to/main.py

It looks like it's failing because it can't write a temp file?

Right, I'm also not sure of this error... but if we were to write the tempfile I think it would still fail. Maybe I'm missing something.

manzt avatar Sep 27 '24 16:09 manzt

I think on Windows we skip checking for a local file, which would result in requestinghttps://example.com/path/to/main.py

OK, so you're referring to the run_url_like_with_local_file_priority test here. I was looking at run_remote_pep723_script.

For run_remote_pep723_script, I think the test should pass.

For run_url_like_with_local_file_priority, right, the thing being tested doesn't make sense on Windows. So I would probably just add a #[cfg(unix)] on that test with a comment explaining why that's there.

BurntSushi avatar Sep 27 '24 16:09 BurntSushi

OK, so you're referring to the run_url_like_with_local_file_priority test here. I was looking at run_remote_pep723_script.

Yes, apologies for the confusion. I added the #[cfg(unix)] with a comment.

manzt avatar Sep 27 '24 17:09 manzt

For run_remote_pep723_script, I think the test should pass.

Agreed.

manzt avatar Sep 27 '24 17:09 manzt

I wonder if the issue on Windows is related to some behavior behind how NamedTempFile works on Windows. Are we "Opening a named temporary file in a world-writable directory."?

manzt avatar Oct 01 '24 20:10 manzt

I wonder if the issue on Windows is related to some behavior behind how NamedTempFile works on Windows. Are we "Opening a named temporary file in a world-writable directory."?

I'm not sure. It seems like it's NamedTempFile::persist that's failing.

Oh, I think I see the problem. You're using write_atomic and passing the path of the temporary file you just created to it. And write_atomic tries to create another temporary file at that same path and persists it. Specifically, I think this is where the error is coming from:

https://github.com/astral-sh/uv/blob/c07000718aa8eb3321f5d46bd454ae863ffc1963/crates/uv-fs/src/lib.rs#L147-L156

I don't think you need write_atomic here. Can you say more about why you used it? I think just a write_all on the NamedTempFile itself is sufficient.

BurntSushi avatar Oct 10 '24 13:10 BurntSushi

Oh, I think I see the problem.

Thanks for explanation. I've learned a lot from this PR.

I don't think you need write_atomic here. Can you say more about why you used it? I think just a write_all on the NamedTempFile itself is sufficient.

It was naive of me. I was looking for how files were written (async) in other parts of the project, but for a tempfile I realize we really don't need. Replaced with write_all!

manzt avatar Oct 10 '24 14:10 manzt

Do we need a file for this? Could we just read into memory and pass the contents over stdin?

charliermarsh avatar Oct 10 '24 14:10 charliermarsh