-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 cached environments in PEP 723 execution #4789
Conversation
a9faa43
to
636d23e
Compare
0abdc3a
to
180562e
Compare
636d23e
to
10838d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think about a plan for cached environment garbage collection
10838d9
to
0949afe
Compare
e92b082
to
84a2da6
Compare
Fixes stack overflows in tests for #4791
84a2da6
to
ab3b621
Compare
@@ -126,4 +126,9 @@ impl CachedEnvironment { | |||
|
|||
Ok(Self(venv)) | |||
} | |||
|
|||
/// Convert the [`EphemeralEnvironment`] into an [`Interpreter`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reintroduced EphemeralEnvironment
after you changed it in #4804. There's a few other places where the comments still need to be changed but given that the file is littered with TODOs I'll just comment here rather than open a PR and create merge conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Summary
This seems like another good candidate for environment caching. If you run a script repeatedly, we can just use the existing cached environment.