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

Pipe input directly in uv run command #8301

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

Conversation

Aditya-PS-05
Copy link
Contributor

closes #6529

Summary

Locally tested, the expected behavior works but some tests are being failed.

@zanieb
Copy link
Member

zanieb commented Oct 17, 2024

Can you title the pull request as a description of the user-facing change? (i.e., as things appear in our changelog)

@Aditya-PS-05 Aditya-PS-05 changed the title Performance/6529 uv run Pipe input directly in uv run command Oct 17, 2024
@Aditya-PS-05
Copy link
Contributor Author

@zanieb ,
Title changed and please review it.

@charliermarsh
Copy link
Member

I'm hesitant to make this change because we still have to read the entire file ourselves in order to find the PEP 723 metadata from the script (which can appear anywhere in the script).

@Aditya-PS-05
Copy link
Contributor Author

@charliermarsh ,
Can you suggest an alternate good method to be implemented?

@zanieb
Copy link
Member

zanieb commented Oct 22, 2024

I'm not sure I agree @charliermarsh, I think it'll be very rare for the metadata to not be at the head of the file and I think it's far more likely that we read the whole file into memory unnecessarily and degrade performance in that way. It's possible the additional complexity here isn't justified, as nobody as complained yet — but I don't think "we could need to read the whole file anyway" is a particularly compelling point against implementing this.

@charliermarsh
Copy link
Member

It’s not that we could need to read the whole file into memory. It’s that we have to scan the whole file, every time, unless it’s actually a PEP 723 script with a comment at the top. Otherwise, we can’t know that the file isn’t a PEP 723 script until we’ve read the whole thing, and we can’t execute it until we know whether it’s a PEP 723 script.

@zanieb
Copy link
Member

zanieb commented Oct 22, 2024

Ah I see. That's a great point. We could just say we don't support inferring that something is a PEP 723 script if it's not in the header, but I see why that's also not appealing given that nobody is complaining about us reading the script into memory today.

@charliermarsh
Copy link
Member

You’re totally right though that if the file is a PEP 723 script and the comment is near the top, we can just buffer until we see it, then steam the rest (and avoid reading the file into memory). So if a lot of the usages here are PEP 723 scripts (which almost certainly have the header near the top), then it would make a difference.

@charliermarsh
Copy link
Member

In that light, I guess I’m not objecting to the behavior in principle, more that it will be complex :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uv run - should pipe input
3 participants