Allow envvar POETRY_PROJECT_PLUGIN_CACHE to override plugin cache path #10618
Conversation
I observed a condition wherein two tests running concurrently with different versions of Python, thus different virtualenv paths, overwrote each others' .poetry/plugins directory since that is managed per-project. Overriding the project plugin cache path with an environment variable set, e.g. `.poetry_310/plugins` and `.poetry_314/plugins` in the two concurrent CI jobs operating on the same working directory, would keep this from happening.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAllow overriding the project’s plugin cache directory via the POETRY_PROJECT_PLUGIN_CACHE environment variable while falling back to the existing .poetry/plugins path. Class diagram for updated ProjectPluginCache path logicclassDiagram
class ProjectPluginCache {
+PATH
+__init__(poetry: Poetry, io: IO)
}
note for ProjectPluginCache "PATH is now set to os.environ.get('POETRY_PROJECT_PLUGIN_CACHE', Path('.poetry') / 'plugins')"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Wrap the environment‐variable value in a Path() constructor so PATH is always a pathlib.Path rather than sometimes a string.
- Since PATH is set at import time, updates to POETRY_PROJECT_PLUGIN_CACHE at runtime (e.g., in tests) won’t be picked up; consider moving the lookup into init or a property so it’s evaluated freshly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Wrap the environment‐variable value in a Path() constructor so PATH is always a pathlib.Path rather than sometimes a string.
- Since PATH is set at import time, updates to POETRY_PROJECT_PLUGIN_CACHE at runtime (e.g., in tests) won’t be picked up; consider moving the lookup into __init__ or a property so it’s evaluated freshly.
## Individual Comments
### Comment 1
<location> `src/poetry/plugins/plugin_manager.py:107` </location>
<code_context>
class ProjectPluginCache:
- PATH = Path(".poetry") / "plugins"
+ PATH = os.environ.get("POETRY_PROJECT_PLUGIN_CACHE", Path(".poetry") / "plugins")
def __init__(self, poetry: Poetry, io: IO) -> None:
</code_context>
<issue_to_address>
**issue:** Mixing Path and str types in os.environ.get default may cause issues.
Since os.environ.get returns a str if the variable is set, PATH may be a str or Path. To avoid type issues, wrap the result with Path(), ensuring PATH is always a Path object.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| class ProjectPluginCache: | ||
| PATH = Path(".poetry") / "plugins" | ||
| PATH = os.environ.get("POETRY_PROJECT_PLUGIN_CACHE", Path(".poetry") / "plugins") |
There was a problem hiding this comment.
issue: Mixing Path and str types in os.environ.get default may cause issues.
Since os.environ.get returns a str if the variable is set, PATH may be a str or Path. To avoid type issues, wrap the result with Path(), ensuring PATH is always a Path object.
There was a problem hiding this comment.
Ugh, I hate it:
| PATH = os.environ.get("POETRY_PROJECT_PLUGIN_CACHE", Path(".poetry") / "plugins") | |
| PATH = Path(os.environ["POETRY_PROJECT_PLUGIN_CACHE"]) if "POETRY_PROJECT_PLUGIN_CACHE" in os.environ else Path(".poetry") / "plugins") |
slightly better:
| PATH = os.environ.get("POETRY_PROJECT_PLUGIN_CACHE", Path(".poetry") / "plugins") | |
| _CACHE_OVERRIDE = "POETRY_PROJECT_PLUGIN_CACHE" | |
| PATH = Path(os.environ[_CACHE_OVERRIDE]) if _CACHE_OVERRIDE in os.environ else Path(".poetry") / "plugins") |
There was a problem hiding this comment.
Another option is to refactor this constant to a static method… that would enable testing it.
There was a problem hiding this comment.
Refactoring the constant to a static method is a solid approach! It makes the logic testable and more flexible for future changes. You could implement it like this:
class ProjectPluginCache:
_CACHE_OVERRIDE = "POETRY_PROJECT_PLUGIN_CACHE"
@staticmethod
def get_path() -> Path:
override = os.environ.get(ProjectPluginCache._CACHE_OVERRIDE)
if override:
return Path(override)
return Path(".poetry") / "plugins"This way, you can easily write tests for different environment variable scenarios, and the type consistency is maintained. If you need to keep a class-level attribute, you could also set PATH = get_path() in __init__ or as a property. Let me know if you'd like a test example or further tweaks!
There was a problem hiding this comment.
@sourcery-ai Refactor usages of ProjectPluginCache.PATH, too.
There was a problem hiding this comment.
Absolutely! If you refactor ProjectPluginCache.PATH to a static method (e.g., get_path()), you'll need to update all usages of ProjectPluginCache.PATH throughout the codebase and tests. For example, any code like:
project_plugin_path = project_path / ProjectPluginCache.PATHshould become:
project_plugin_path = project_path / ProjectPluginCache.get_path()This change will ensure that the plugin cache path is always computed dynamically, making it easier to test and to respect the environment variable override. You can search for all usages of ProjectPluginCache.PATH in your codebase and update them accordingly. If you need help with a specific usage or want a test example for the new method, let me know!
There was a problem hiding this comment.
@sourcery-ai I hoped you would provide a comment with a suggestion code block ;-)
There was a problem hiding this comment.
Here's a suggestion for refactoring usages of ProjectPluginCache.PATH to use a static method get_path(). This will ensure all references dynamically compute the path and respect the environment variable override.
First, update the class definition:
class ProjectPluginCache:
_CACHE_OVERRIDE = "POETRY_PROJECT_PLUGIN_CACHE"
@staticmethod
def get_path() -> Path:
override = os.environ.get(ProjectPluginCache._CACHE_OVERRIDE)
if override:
return Path(override)
return Path(".poetry") / "plugins"Then, update all usages of ProjectPluginCache.PATH to ProjectPluginCache.get_path():
src/poetry/plugins/plugin_manager.py
plugin_path = pyproject_toml.parent / ProjectPluginCache.get_path()tests/console/test_application.py
project_plugin_path = project_path / ProjectPluginCache.get_path()tests/plugins/test_plugin_manager.py
assert not (poetry.pyproject_path.parent / ProjectPluginCache.get_path()).exists()
# ... and similar for other usagesThis approach keeps the logic centralized and testable. If you need a more detailed diff or want to see how to update a specific test, let me know!
Added information about changing the plugin cache path.
|
I'm planning to refactor this to put the path in a static method but I need a little more IDE than single-file editing. My work VPN blocks Codespaces and I'm too close to the end of the day to set things up locally. |
Only Poetry's own Python version should be relevant, not the project's Python version. Do you use two different Poetry installations? |
In this situation, yes, two different Poetry installations in separate containers running concurrently against the same project file system. |
|
I wonder if this is too much of an edge case. (I am not strictly opposed. I just want to understand if it is worth it.) Why not installing the required plugins together with Poetry in the container? Is a command to install the plugins that much more inconvenient than setting an environment variable? |
2 participants
I observed a condition wherein two tests running concurrently with different versions of Python, thus different virtualenv paths, overwrote each others' .poetry/plugins directory since that is managed per-project.
Overriding the project plugin cache path with an environment variable set, e.g.
.poetry_310/pluginsand.poetry_314/pluginsin the two concurrent CI jobs operating on the same working directory, would keep this from happening.Pull Request Check List
Resolves: (None)
Summary by Sourcery
New Features: