Cache dependency version fetches per invocation#552
Conversation
Memoize the requests-cache session factory and each dependency's _fetch_versions helper with functools.cache so the per-process backend init and JSON fetch+parse happen exactly once regardless of how many DependencyConstraint instances ask for them. Previously each PythonDependencyConstraint, RDependencyConstraint, etc. opened a fresh CachedSession and walked through the local HTTP cache on every resolve_versions call, producing repeated "Initializing backend" and "Cache directives" DEBUG lines and redundant parse work. Closes #330
bschwedler
force-pushed
the
cache-dependency-fetches
branch
from
92fdf16 to
dcab125
Compare
There was a problem hiding this comment.
Pull request overview
This PR reduces redundant dependency-version retrieval during a bakery invocation by memoizing the shared cached HTTP session and dependency version fetch helpers.
Changes:
- Memoizes
cached_session()so the requests-cache backend initializes once per process. - Converts dependency
_fetch_versionshelpers to cached static methods. - Clears the new caches in test fixtures so mocked sessions remain isolated per test.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
posit-bakery/posit_bakery/util.py |
Adds process-wide memoization for the cached requests session. |
posit-bakery/posit_bakery/config/dependencies/python.py |
Memoizes Python version fetch/parse work. |
posit-bakery/posit_bakery/config/dependencies/r.py |
Memoizes R version fetch/parse work. |
posit-bakery/posit_bakery/config/dependencies/quarto.py |
Memoizes Quarto version fetches per prerelease mode. |
posit-bakery/posit_bakery/config/dependencies/positron.py |
Memoizes Positron version fetch/parse work. |
posit-bakery/test/config/conftest.py |
Clears memoized caches before request-mocking fixtures run. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @cache | ||
| def cached_session() -> CachedSession: |
There was a problem hiding this comment.
We shouldn't need to test core language functionality IMO. This isn't a complicated setup.
| return CachedSession( | ||
| cache_name="bakery_cache", | ||
| expire_after=3600, | ||
| backend="filesystem", | ||
| use_temp=True, | ||
| allowable_methods=["GET"], | ||
| allowable_codes=[200], | ||
| stale_if_error=True, | ||
| ) |
There was a problem hiding this comment.
I have a crazy idea: what if we cached this somehow as part of setup-bakery? Give it some sort of reasonable TTL and then recall the artifact it creates across all jobs in a workflow. That way we can resolve #304 and edge cases where new versions can crop up between build time and merge time.
| @cache | ||
| def cached_session() -> CachedSession: |
There was a problem hiding this comment.
We shouldn't need to test core language functionality IMO. This isn't a complicated setup.
|
|
||
| def _fetch_versions(self) -> list[DependencyVersion]: | ||
| @staticmethod | ||
| @cache |
There was a problem hiding this comment.
roborev noted that the cached result can be mutated. We may want to note that or find a way to always return a copy.
| @@ -92,6 +92,21 @@ def patch_testdata_response(url: str): | |||
|
|
|||
| @pytest.fixture(scope="function") | |||
| def disable_requests_caching(mocker): | |||
There was a problem hiding this comment.
Should this be autouse?
Closes #330
Memoize the requests-cache session factory and each dependency's
_fetch_versionshelper so backend init and JSON fetch+parse run exactly once perbakeryinvocation, instead of once perDependencyConstraintinstance.