You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by "kotborealis (via GitHub)" <gi...@apache.org> on 2026/08/12 14:26:42 UTC
[PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
kotborealis opened a new pull request, #2172:
URL: https://github.com/apache/buildstream/pull/2172
This change removes redundant work from artifact creation, remote CAS directory fetching, and cache-key calculation:
- Remove the unused recursive artifact size calculation.
- Avoid issuing a second `FetchTree` request in `CASCache.fetch_directory()`.
- Reuse the build dependency list when calculating weak and strict cache keys.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#issuecomment-5316466403
> The failed CI jobs are all failing for the same unrelated reason.
>
> `bst init` still uses `2.7` as the default `--min-version` in `src/buildstream/_frontend/cli.py:423`, while the build is detected as BuildStream `2.8.0.dev0`.
https://github.com/apache/buildstream/pull/2175 Fixes this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "kotborealis (via GitHub)" <gi...@apache.org>.
kotborealis commented on PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#issuecomment-5316243544
The failed CI jobs are all failing for the same unrelated reason.
`bst init` still uses `2.7` as the default `--min-version` in `src/buildstream/_frontend/cli.py:423`, while the build is detected as BuildStream `2.8.0.dev0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim merged PR #2172:
URL: https://github.com/apache/buildstream/pull/2172
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "kotborealis (via GitHub)" <gi...@apache.org>.
kotborealis commented on code in PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#discussion_r3796521579
##########
src/buildstream/element.py:
##########
@@ -3337,6 +3337,8 @@ def __update_cache_keys(self):
# This code can be run multiple times until the strict key can be calculated,
# so let's ensure we only ever calculate the weak key once, even though we need
# to resolve it before we can resolve the strict key.
+ build_dependencies = list(self._dependencies(_Scope.BUILD))
Review Comment:
I agree that caching `_dependencies()` could provide a better optimization, but for this PR I kept the change deliberately narrow.
A cache would need careful invalidation when dependencies are added through.
I would prefer to handle that broader refactoring separately, with dedicated tests. The current change only targets the duplicate traversal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#discussion_r3870745595
##########
src/buildstream/_cas/cascache.py:
##########
@@ -286,7 +286,7 @@ def fetch_directory(self, remote, dir_digest):
"Failed to fetch directory tree {}: {}: {}".format(dir_digest.hash, e.code().name, e.details())
) from e
- required_blobs = self.required_blobs_for_directory(dir_digest)
+ required_blobs = self.required_blobs_for_directory(dir_digest, _fetch_tree=False)
Review Comment:
I wonder if it would be better to drop the fetch tree from this function, and rely on the one in `required_blobs_for_directory()`. I'll try to dig deeper.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#discussion_r3795555037
##########
src/buildstream/element.py:
##########
@@ -3337,6 +3337,8 @@ def __update_cache_keys(self):
# This code can be run multiple times until the strict key can be calculated,
# so let's ensure we only ever calculate the weak key once, even though we need
# to resolve it before we can resolve the strict key.
+ build_dependencies = list(self._dependencies(_Scope.BUILD))
Review Comment:
Just to note:
I've proposed a refactor of _calculate_cache_key over here: https://github.com/apache/buildstream/pull/2167/changes/c3369b7577d9ea32862512c9b38c844cbad38bc5#diff-7d3ea8e226c37028881ae2f47facfe404a1c84346d4238569a63b0caacfb0ea2R2360 To avoid the complicated list comprehension that goes on here, with it's mess of dynamic typing of list[tuple[str,str] | tuple[str,str,None] | tuple[str,str,str]] and hard to read logic.
Although, I still end up calling _dependencies every time _calculate_cache_key is called...
-----
Feedback:
hmm.. Instead of storing the result of _dependencies here, it might be worth refactoring `_dependencies` to directly cache it's own results, so it can return the cached result after the first call, whenever it's called anywhere instead of just here.
```python
...
__dependency_set_cache: dict[str,set[Element]] = {}
def __dependencies(...) -> ...:
# Check if we already calculated this set of dependencies
dependency_set_cache_key = f"{scope}, {recurse}" # (probably need to include the _dependencies 'visited' argument here too...)
if (dependencies := self.__dependency_set_cache.get(dependency_set_cache_key)) is not None:
for element in dependencies:
yield element
return
# Calculate result/visited
....
# Store result for next time
self.__build_dependencies_set[dependency_set_cache_key] = result # ( or 'visited' from the recursive visit function)
```
It will need some thinking, but maybe it would even be appropriate to use functools `@cache` annotation? https://docs.python.org/3/library/functools.html The recursive visit function might also benefit from the @cache annotation..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#discussion_r3796568589
##########
src/buildstream/element.py:
##########
@@ -3337,6 +3337,8 @@ def __update_cache_keys(self):
# This code can be run multiple times until the strict key can be calculated,
# so let's ensure we only ever calculate the weak key once, even though we need
# to resolve it before we can resolve the strict key.
+ build_dependencies = list(self._dependencies(_Scope.BUILD))
Review Comment:
Do you want to open a follow-up issue for this then and we can close this thread?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#discussion_r3906651188
##########
src/buildstream/_cas/cascache.py:
##########
@@ -286,7 +286,7 @@ def fetch_directory(self, remote, dir_digest):
"Failed to fetch directory tree {}: {}: {}".format(dir_digest.hash, e.code().name, e.details())
) from e
- required_blobs = self.required_blobs_for_directory(dir_digest)
+ required_blobs = self.required_blobs_for_directory(dir_digest, _fetch_tree=False)
Review Comment:
Actually, my comment isn't correct. In this function we fetch from the remote but `required_blobs_for_directory()` only tries to fetch from the cache storage-service if it's configured.
So this is indeed an optimisation, but only for the specific use case when using a storage-service for the cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#discussion_r3795555037
##########
src/buildstream/element.py:
##########
@@ -3337,6 +3337,8 @@ def __update_cache_keys(self):
# This code can be run multiple times until the strict key can be calculated,
# so let's ensure we only ever calculate the weak key once, even though we need
# to resolve it before we can resolve the strict key.
+ build_dependencies = list(self._dependencies(_Scope.BUILD))
Review Comment:
Just to note:
I've proposed a refactor of _calculate_cache_key over here: https://github.com/apache/buildstream/pull/2167/changes/c3369b7577d9ea32862512c9b38c844cbad38bc5#diff-7d3ea8e226c37028881ae2f47facfe404a1c84346d4238569a63b0caacfb0ea2R2360 To avoid the complicated list comprehension that goes on here, with it's mess of dynamic typing of list[tuple[str,str] | tuple[str,str,None] | tuple[str,str,str]] and hard to read logic.
Although, I still end up calling _dependencies every time _calculate_cache_key is called...
-----
Feedback:
hmm.. Instead of storing the result of _dependencies here, it might be worth refactoring `_dependencies` to directly cache it's own results, so it can return the cached result after the first call, whenever it's called anywhere instead of just here. I don't think __build_dependencies or __runtime_dependencies is ever updated after an Element is first initialised in _new_from_load_element. (Although _add_build_dependency might be a problem? that would need investigating, but you could clear the cache, in that method..)
```python
...
__dependency_set_cache: dict[str,set[Element]] = {}
def __dependencies(...) -> ...:
# Check if we already calculated this set of dependencies
dependency_set_cache_key = f"{scope}, {recurse}" # (probably need to include the _dependencies 'visited' argument here too...)
if (dependencies := self.__dependency_set_cache.get(dependency_set_cache_key)) is not None:
for element in dependencies:
yield element
return
# Calculate result/visited
....
# Store result for next time
self.__build_dependencies_set[dependency_set_cache_key] = result # ( or 'visited' from the recursive visit function)
```
It will need some thinking, but maybe it would even be appropriate to use functools `@cache` annotation? https://docs.python.org/3/library/functools.html The recursive visit function might also benefit from the @cache annotation..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "kotborealis (via GitHub)" <gi...@apache.org>.
kotborealis commented on code in PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#discussion_r3796659493
##########
src/buildstream/element.py:
##########
@@ -3337,6 +3337,8 @@ def __update_cache_keys(self):
# This code can be run multiple times until the strict key can be calculated,
# so let's ensure we only ever calculate the weak key once, even though we need
# to resolve it before we can resolve the strict key.
+ build_dependencies = list(self._dependencies(_Scope.BUILD))
Review Comment:
#2176
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "kotborealis (via GitHub)" <gi...@apache.org>.
kotborealis commented on code in PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#discussion_r3871692236
##########
src/buildstream/_cas/cascache.py:
##########
@@ -286,7 +286,7 @@ def fetch_directory(self, remote, dir_digest):
"Failed to fetch directory tree {}: {}: {}".format(dir_digest.hash, e.code().name, e.details())
) from e
- required_blobs = self.required_blobs_for_directory(dir_digest)
+ required_blobs = self.required_blobs_for_directory(dir_digest, _fetch_tree=False)
Review Comment:
Maybe we could merge this for the 2.8 release?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#issuecomment-5316736334
> > The failed CI jobs are all failing for the same unrelated reason.
> > `bst init` still uses `2.7` as the default `--min-version` in `src/buildstream/_frontend/cli.py:423`, while the build is detected as BuildStream `2.8.0.dev0`.
>
> #2175 Fixes this.
PR merged, you should be able to rebase the branch for a happy CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#discussion_r3795555037
##########
src/buildstream/element.py:
##########
@@ -3337,6 +3337,8 @@ def __update_cache_keys(self):
# This code can be run multiple times until the strict key can be calculated,
# so let's ensure we only ever calculate the weak key once, even though we need
# to resolve it before we can resolve the strict key.
+ build_dependencies = list(self._dependencies(_Scope.BUILD))
Review Comment:
Just to note:
I've proposed a refactor of _calculate_cache_key over here: https://github.com/apache/buildstream/pull/2167/changes/c3369b7577d9ea32862512c9b38c844cbad38bc5#diff-7d3ea8e226c37028881ae2f47facfe404a1c84346d4238569a63b0caacfb0ea2R2360 To avoid the complicated list comprehension that goes on here, with it's mess of dynamic typing of list[tuple[str,str] | tuple[str,str,None] | tuple[str,str,str]] and hard to read logic.
Although, I still end up calling _dependencies every time _calculate_cache_key is called...
-----
Feedback:
hmm.. Instead of storing the result of _dependencies here, it might be worth refactoring `_dependencies` to directly cache it's own results, so it can return the cached result after the first call, whenever it's called anywhere instead of just here. I don't think __build_dependencies or __runtime_dependencies is ever updated after an Element is first initialised in _new_from_load_element. (Although _add_build_dependency might be a problem? that would need investigating)
```python
...
__dependency_set_cache: dict[str,set[Element]] = {}
def __dependencies(...) -> ...:
# Check if we already calculated this set of dependencies
dependency_set_cache_key = f"{scope}, {recurse}" # (probably need to include the _dependencies 'visited' argument here too...)
if (dependencies := self.__dependency_set_cache.get(dependency_set_cache_key)) is not None:
for element in dependencies:
yield element
return
# Calculate result/visited
....
# Store result for next time
self.__build_dependencies_set[dependency_set_cache_key] = result # ( or 'visited' from the recursive visit function)
```
It will need some thinking, but maybe it would even be appropriate to use functools `@cache` annotation? https://docs.python.org/3/library/functools.html The recursive visit function might also benefit from the @cache annotation..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] reduce redundant artifact, CAS, and cache-key work [buildstream]
Posted by "nathanwilliams-ct (via GitHub)" <gi...@apache.org>.
nathanwilliams-ct commented on code in PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#discussion_r3795555037
##########
src/buildstream/element.py:
##########
@@ -3337,6 +3337,8 @@ def __update_cache_keys(self):
# This code can be run multiple times until the strict key can be calculated,
# so let's ensure we only ever calculate the weak key once, even though we need
# to resolve it before we can resolve the strict key.
+ build_dependencies = list(self._dependencies(_Scope.BUILD))
Review Comment:
Just to note:
I've proposed a refactor of _calculate_cache_key over here: https://github.com/apache/buildstream/pull/2167/changes/c3369b7577d9ea32862512c9b38c844cbad38bc5#diff-7d3ea8e226c37028881ae2f47facfe404a1c84346d4238569a63b0caacfb0ea2R2360 To avoid the complicated list comprehension that goes on here, with it's mess of dynamic typing of list[tuple[str,str] | tuple[str,str,None] | tupe[str,str,str]] and hard to read logic.
Although, I still end up calling _dependencies every time _calculate_cache_key is called...
-----
Feedback:
hmm.. Instead of storing the result of _dependencies here, it might be worth refactoring `_dependencies` to directly cache it's own results, so it can return the cached result after the first call, whenever it's called anywhere instead of just here.
```python
...
__dependency_set_cache: dict[str,set[Element]] = {}
def __dependencies(...) -> ...:
# Check if we already calculated this set of dependencies
dependency_set_cache_key = f"{scope}, {recurse}" # (probably need to include the _dependencies 'visited' argument here too...)
if (dependencies := self.__dependency_set_cache.get(dependency_set_cache_key)) is not None:
for element in dependencies:
yield element
return
# Calculate result/visited
....
# Store result for next time
self.__build_dependencies_set[dependency_set_cache_key] = result # ( or 'visited' from the recursive visit function)
```
It will need some thinking, but maybe it would even be appropriate to use functools `@cache` annotation? https://docs.python.org/3/library/functools.html The recursive visit function might also benefit from the @cache annotation..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org