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

avoid duplicate log statements by memoizing getters #1248

Merged
merged 3 commits into from Mar 28, 2023

Conversation

minrk
Copy link
Member

@minrk minrk commented Feb 16, 2023

ensures get_ functions are called only once, avoiding duplicate log statements. Closes #1246 .

This may be too clever to do it magically on methods that start with get_, but it works and is the simplest if we want to clearly always cache all these methods (I think we do).

The alternatives are:

  1. more standard, far more changes (both now and over time) - apply @lru_cache() on every method, including every subclass method override (easy to miss, as the only downside of forgetting it is duplicate log statements and a negligible amount of wasted time)
  2. to ensure these methods are called only once on the parent class, e.g. by assigning the results to variables, or calling directly via an additional private method self._get_.... This is ultimately the same as the memoize here, but implemented with explicit private methods instead of caching all public methods based on their name.
  3. like 2., but only on the functions we expect to call more than once (get_..._files() are called in build and again in render)

ensures `get_` functions are called only once,
avoiding duplicate log statements
@minrk minrk added the bug label Feb 16, 2023
@minrk minrk changed the title memoize getters avoid duplicate log statements by memoizing getters Feb 16, 2023
Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is super clever, this makes me somewhat uncomfortable - I can imagine myself up at 3am some day, looking for reasons why my get_somethingelse method is just not being called. I'd much prefer alternative 1, with lru being decorated for each of them...

rather than automatically
@minrk
Copy link
Member Author

minrk commented Mar 23, 2023

Fair enough! Now @lru_cache is applied to all get methods explicitly

@yuvipanda yuvipanda merged commit b52ea66 into jupyterhub:main Mar 28, 2023
17 of 18 checks passed
@yuvipanda
Copy link
Collaborator

Thanks @minrk

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.

duplicate logs about environment
2 participants