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

Documentation of lru_cache-able methods #468

Closed
JCGoran opened this issue Nov 22, 2022 · 1 comment
Closed

Documentation of lru_cache-able methods #468

JCGoran opened this issue Nov 22, 2022 · 1 comment

Comments

@JCGoran
Copy link
Contributor

JCGoran commented Nov 22, 2022

Problem Description

This is not a bug per se, but I'm not sure what the "recommended" way to resolve the situation is.
Basically, there is a slight problem when one wants to use the lru_cache decorator from the functools module on instance methods (described in more detail here), in that one should not do this:

class ExpensiveComputation:
    def __init__(self, ...):

    @lru_cache(maxsize=None)
    def calculate(self, ...):
        ...

but rather, something like this:

class ExpensiveComputation:
    def __init__(self, ...):
        ...
        self.calculate = lru_cache(maxsize=None)(self._calculate)

    def _calculate(self, ...):
        ...

The problem with the above is that pdoc does not document methods that begin with underscores, meaning there are at least 3 not-so-great ways to document the calculate method in the fixed version from above:

  1. leave the docstring where it is; this means that the method is not documented at all (probably not the best)
  2. rename _calculate to something that doesn't begin with an underscore and write a warning in the docstring to call calculate instead; this is a bit strange to both end users and myself
  3. move the whole docstring to self.calculate (the now-member); the downsides of this are a) the full signature of the function is lost and needs to be documented, b) there is no "View Source" next to the method (since pdoc considers it a member), and c) it feels a bit strange to have such an enormous docstring in the middle of the constructor

So far, option 3 looks like the most sane one, but none of them are really "nice" (admittedly, the whole workaround for lru_cache is also a bit inelegant).

Steps to reproduce the behavior:

First (wrong) implementation:

import time
from functools import lru_cache


class ExpensiveComputation:
    def __init__(self, delay: int = 1):
        """
        Constructor

        Parameters
        ----------
        delay, optional
            the delay
        """
        self.delay = delay

    @lru_cache(maxsize=None)
    def calculate(self, arg: float) -> float:
        """
        Function for calculation

        Parameters
        ----------
        arg
            the parameter to square

        Returns
        -------
        float
            the result
        """
        time.sleep(self.delay)
        return arg**2

Rendering:
image

Second (fixed) implementation, with docstring fix from option 3:

import time
from functools import lru_cache


class ExpensiveComputation:
    def __init__(self, delay: int = 1):
        """
        Constructor

        Parameters
        ----------
        delay, optional
            the delay
        """

        self.delay = delay
        self.calculate = lru_cache(maxsize=None)(self._calculate)
        """
        Function for calculation

        Parameters
        ----------
        arg
            the parameter to square

        Returns
        -------
        float
            the result
        """

    def _calculate(self, arg: float) -> float:
        """
        Function for calculation

        Parameters
        ----------
        arg
            the parameter to square

        Returns
        -------
        float
            the result
        """
        time.sleep(self.delay)
        return arg**2

Rendering:
image

I've generated the above using the --docformat numpy flag, but it's not necessary to reproduce the behavior.

System Information

pdoc: 12.3.0
Python: 3.9.13
Platform: Linux-5.16.0-1-amd64-x86_64-with-glibc2.35
@JCGoran JCGoran added the bug label Nov 22, 2022
@mhils
Copy link
Member

mhils commented Nov 22, 2022

For this specific use case, I would probably just do the following:

class CachedInstanceMethod:
    def __init__(self):
        self.expensive_computation = functools.cache(self.expensive_computation)
    
    # @cache added in __init__
    def expensive_computation(self) -> None:
        """Here is a docstring."""

The comment makes sure that a future reader who skips __init__ still knows that caching is happening, which would otherwise be my main complaint about this solution.

@mhils mhils removed the bug label Dec 3, 2022
@mhils mhils closed this as completed Jan 21, 2024
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

No branches or pull requests

2 participants