perf: cache absolute scheme paths and avoid is_relative_to#10958
Open
radoering wants to merge 1 commit into
Open
perf: cache absolute scheme paths and avoid is_relative_to#10958radoering wants to merge 1 commit into
is_relative_to#10958radoering wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
_abspath_scheme_cachetype hint declaresdict[str, str]but is keyed byScheme, so it would be clearer and more accurate to annotate it asdict[Scheme, str](or similar) to match actual usage. - The
startswith(target_dir + os.sep)check may behave unexpectedly on case-insensitive filesystems (e.g. Windows); consider normalizing withos.path.normcaseor similar on bothtarget_dirandtarget_path_strbefore comparison to avoid rejecting valid paths due to case differences.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_abspath_scheme_cache` type hint declares `dict[str, str]` but is keyed by `Scheme`, so it would be clearer and more accurate to annotate it as `dict[Scheme, str]` (or similar) to match actual usage.
- The `startswith(target_dir + os.sep)` check may behave unexpectedly on case-insensitive filesystems (e.g. Windows); consider normalizing with `os.path.normcase` or similar on both `target_dir` and `target_path_str` before comparison to avoid rejecting valid paths due to case differences.
## Individual Comments
### Comment 1
<location path="src/poetry/installation/wheel_installer.py" line_range="37-38" />
<code_context>
class WheelDestination(SchemeDictionaryDestination):
""" """
+ @cached_property
+ def _abspath_scheme_cache(self) -> dict[str, str]:
+ return {}
+
</code_context>
<issue_to_address>
**issue:** Cache dict type annotation does not match actual key type used (`scheme`).
Since `scheme` is a `Scheme`, but the cache is typed as `dict[str, str]` while being accessed via `cache.get(scheme)` / `cache[scheme]`, this will cause type-checker warnings and obscures intent. Please update the annotation to use `Scheme` as the key type (e.g. `dict[Scheme, str]`) to match actual usage.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
I had to think twice about it, but we do not need this because both paths are built from |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
_abspath_scheme_cachetype annotation (dict[str, str]) does not match how it’s used (cache[scheme]whereschemeis aScheme), so consider tightening this todict[Scheme, str](or the appropriate key type) to keep static typing accurate. - Since
_abspath_scheme_cacheis a mutable dict returned from acached_property, it may be clearer to skip the property entirely and initialize the cache once in__init__(e.g.,self._abspath_scheme_cache: dict[Scheme, str] = {}) to avoid surprising mutation of a property.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_abspath_scheme_cache` type annotation (`dict[str, str]`) does not match how it’s used (`cache[scheme]` where `scheme` is a `Scheme`), so consider tightening this to `dict[Scheme, str]` (or the appropriate key type) to keep static typing accurate.
- Since `_abspath_scheme_cache` is a mutable dict returned from a `cached_property`, it may be clearer to skip the property entirely and initialize the cache once in `__init__` (e.g., `self._abspath_scheme_cache: dict[Scheme, str] = {}`) to avoid surprising mutation of a property.
## Individual Comments
### Comment 1
<location path="src/poetry/installation/wheel_installer.py" line_range="83-84" />
<code_context>
+
+ # We do not need os.path.normcase() for this comparison
+ # because both paths are built from target_dir.
+ if target_path_str != target_dir and not target_path_str.startswith(
+ target_dir + os.sep
+ ):
raise ValueError(
</code_context>
<issue_to_address>
**issue (bug_risk):** The startswith() directory check misbehaves when the target directory is a filesystem root (e.g. '/' or 'C:\').
This `target_path_str != target_dir and not target_path_str.startswith(target_dir + os.sep)` condition breaks for root-like directories. For `target_dir == '/'`, `target_dir + os.sep` is `'//'`, so a valid path like `'/usr'` is rejected. On Windows, `'C:\' + os.sep` becomes `'C:\\'`, so `'C:\Users'` also fails. Consider using a path-based containment check such as `os.path.commonpath([target_dir, target_path_str]) == target_dir`, or normalizing `target_dir` for the prefix comparison so that roots are handled correctly (e.g. only trimming a trailing separator when `len(target_dir) > 1`, then appending `os.sep`).
</issue_to_address>
### Comment 2
<location path="src/poetry/installation/wheel_installer.py" line_range="37-38" />
<code_context>
class WheelDestination(SchemeDictionaryDestination):
""" """
+ @cached_property
+ def _abspath_scheme_cache(self) -> dict[str, str]:
+ return {}
+
</code_context>
<issue_to_address>
**suggestion:** The type annotation for the cache does not match how it is used as a dictionary keyed by Scheme.
Since `scheme` is a `Scheme` when used in `cache.get(scheme)`, this cache is logically a `dict[Scheme, str]`, not `dict[str, str]`. Please update the annotation (or use an appropriate alias) so it matches actual usage and avoids type-checker errors.
Suggested implementation:
```python
@cached_property
def _abspath_scheme_cache(self) -> dict["Scheme", str]:
return {}
```
This change assumes that `Scheme` is already imported or defined in this module (or will be). If it is not yet available, you should:
1. Import `Scheme` in this file (potentially under a `TYPE_CHECKING` block if it is only needed for type hints), for example:
```python
if TYPE_CHECKING:
from some.module import Scheme
```
2. Adjust the import path (`some.module`) to the actual location of the `Scheme` type in your codebase.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
scheme paths do not change during installation. Thus, we should determine their absolute paths only once. `Path.is_relative_to` adds a significant overhead compared to a simple startswith check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scheme paths do not change during installation. Thus, we should determine their absolute paths only once.
Path.is_relative_toadds a significant overhead compared to a simple startswith check.