Skip to content

fix(lint): remove functools.cache from methods#1684

Merged
dlovell merged 3 commits intomainfrom
fix/b019-cached-property
Mar 12, 2026
Merged

fix(lint): remove functools.cache from methods#1684
dlovell merged 3 commits intomainfrom
fix/b019-cached-property

Conversation

@mesejo
Copy link
Collaborator

@mesejo mesejo commented Mar 5, 2026

  • Replace @property @functools.cache combos with @functools.cached_property to fix B019 violations and eliminate memory leaks from instance-keyed caches
  • Convert standalone @functools.cache methods (copy_sdist) to @functools.cached_property and update call sites accordingly
  • Replace class-level cached_property aliases (popened = _uv_build_popened) with explicit @property delegates (Python 3.13 disallows reusing the same cached_property object under two names)
  • Add # noqa: B019 for make_deferred_other, which takes extra arguments and cannot be converted to a cached_property

Fix a latent bug in SdistBuilder.maybe_packager: the field had converter=toolz.curried.excepts(Exception, Path), which silently converted a Sdister object to None because Path(sdister_instance) raises TypeError. This caused the Sdister to be garbage-collected immediately after SdistBuilder.from_script_path returned, cleaning up its
TemporaryDirectory and deleting the sdist file that SdistBuilder.sdist_path pointed to. Subsequent access to sdist_path in _uv_tool_run_xorq_build then failed with FileNotFoundError. The fix removes the broken converter, so maybe_packager holds the Sdister directly, keeping it alive for the lifetime of the SdistBuilder.

Proof that @frozen (attrs) works with @cached_property:

from attrs import frozen
from functools import cached_property

@Frozen
class Circle:
    radius: float

    @cached_property
    def area(self):
        print("computing...")
        return 3.14159 * self.radius ** 2

c = Circle(radius=5.0)
print(c.area)  # computing... → 78.53975
print(c.area)  # cached → 78.53975

@github-actions github-actions bot added the fix label Mar 5, 2026
@mesejo mesejo changed the title fix(lint): remove @functools.cache from methods fix(lint): remove functools.cache from methods Mar 5, 2026
@codecov
Copy link

codecov bot commented Mar 5, 2026

Codecov Report

❌ Patch coverage is 96.22642% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
python/xorq/ibis_yaml/packager.py 89.47% 2 Missing ⚠️
Files with missing lines Coverage Δ
python/xorq/catalog/tui.py 70.16% <100.00%> (-0.36%) ⬇️
python/xorq/common/utils/process_utils.py 94.93% <100.00%> (-0.19%) ⬇️
python/xorq/common/utils/tar_utils.py 92.02% <100.00%> (-0.34%) ⬇️
...thon/xorq/common/utils/tests/test_deferred_read.py 96.75% <100.00%> (-0.03%) ⬇️
python/xorq/expr/ml/fit_lib.py 88.77% <100.00%> (ø)
python/xorq/expr/ml/pipeline_lib.py 90.09% <100.00%> (-0.12%) ⬇️
python/xorq/expr/ml/sklearn_utils.py 88.57% <100.00%> (-0.32%) ⬇️
python/xorq/flight/__init__.py 92.56% <100.00%> (-0.05%) ⬇️
python/xorq/ibis_yaml/compiler.py 95.41% <100.00%> (-0.04%) ⬇️
python/xorq/ibis_yaml/packager.py 87.55% <89.47%> (-1.03%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 5, 2026

Merging this PR will not alter performance

✅ 14 untouched benchmarks


Comparing fix/b019-cached-property (cbcf087) with main (f9d19bf)

Open in CodSpeed

@mesejo mesejo force-pushed the fix/b019-cached-property branch 2 times, most recently from 80960b9 to 37f122f Compare March 11, 2026 09:40
@mesejo mesejo marked this pull request as ready for review March 11, 2026 09:44
@mesejo mesejo force-pushed the fix/b019-cached-property branch 2 times, most recently from 67a9379 to 36b652c Compare March 11, 2026 14:35
@mesejo mesejo requested a review from dlovell March 11, 2026 14:49
mesejo and others added 3 commits March 12, 2026 05:15
…property (B019)

- Replace `@property @functools.cache` combos with `@functools.cached_property`
  to fix B019 violations and eliminate memory leaks from instance-keyed caches
- Convert standalone `@functools.cache` methods (`copy_sdist`) to
  `@functools.cached_property` and update call sites accordingly
- Replace class-level `cached_property` aliases (`popened = _uv_build_popened`)
  with explicit `@property` delegates (Python 3.13 disallows reusing the same
  `cached_property` object under two names)
- Add `# noqa: B019` for `make_deferred_other`, which takes extra arguments
  and cannot be converted to a `cached_property`

Fix a latent bug in `SdistBuilder.maybe_packager`: the field had
`converter=toolz.curried.excepts(Exception, Path)`, which silently converted
a `Sdister` object to `None` because `Path(sdister_instance)` raises
`TypeError`. This caused the `Sdister` to be garbage-collected immediately
after `SdistBuilder.from_script_path` returned, cleaning up its
`TemporaryDirectory` and deleting the sdist file that `SdistBuilder.sdist_path`
pointed to. Subsequent access to `sdist_path` in `_uv_tool_run_xorq_build`
then failed with `FileNotFoundError`. The fix removes the broken converter so
`maybe_packager` holds the `Sdister` directly, keeping it alive for the
lifetime of the `SdistBuilder`.

Proof that `@frozen` (attrs) works with `@cached_property`:

    from attrs import frozen
    from functools import cached_property

    @Frozen
    class Circle:
        radius: float

        @cached_property
        def area(self):
            print("computing...")
            return 3.14159 * self.radius ** 2

    c = Circle(radius=5.0)
    print(c.area)  # computing... → 78.53975
    print(c.area)  # cached → 78.53975

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@mesejo mesejo force-pushed the fix/b019-cached-property branch from 6d41455 to cbcf087 Compare March 12, 2026 04:17
@mesejo mesejo requested a review from dlovell March 12, 2026 09:35
@dlovell dlovell merged commit 160b635 into main Mar 12, 2026
25 checks passed
@dlovell dlovell deleted the fix/b019-cached-property branch March 12, 2026 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants