abderrahim commented on code in PR #2031:
URL: https://github.com/apache/buildstream/pull/2031#discussion_r2188277955
##########
src/buildstream/_remote.py:
##########
@@ -72,10 +71,6 @@ def init(self):
self._initialized = True
def close(self):
- if self.channel:
- self.channel.close()
- self.channel = None
-
self._initialized = False
Review Comment:
Is there any value in keeping the `close()` method now? AFAICT, there is no
more cleanup to do now that everything goes through buildbox-casd.
##########
requirements/requirements.in:
##########
@@ -4,7 +4,7 @@ Jinja2 >= 2.10
importlib_metadata >= 3.6; python_version < "3.10"
packaging
pluginbase
-protobuf <6.0dev,>=5.26.1
+protobuf <6.0dev,>=5.29
Review Comment:
Personal preference, but I like to have updating .proto files and updating
the grpcio-tools version that generates them in different commits.
##########
src/buildstream/sandbox/_sandboxbuildboxrun.py:
##########
@@ -32,6 +34,27 @@
# BuildBox-based sandbox implementation.
#
class SandboxBuildBoxRun(SandboxREAPI):
+ def __init__(self, *args, **kwargs):
+ super().__init__(*args, **kwargs)
+
+ context = self._get_context()
+ cascache = context.get_cascache()
+
+ re_specs = context.remote_execution_specs
+ if re_specs and re_specs.action_spec and not re_specs.exec_spec:
Review Comment:
Is there a value in excluding this when we have `re_specs.exec_spec`? IIUC,
we can use this sandbox even though remote-execution is configured in code
paths that go through `Element._prepare_sandbox()` (which sets
allow_remote=False) such as `bst shell`.
Is there a reason to prevent using the action cache in that situation?
##########
src/buildstream/sandbox/_sandboxbuildboxrun.py:
##########
@@ -91,8 +114,10 @@ def _execute_action(self, action, flags):
casd = cascache.get_casd()
config = self._get_config()
- if config.remote_apis_socket_path and context.remote_cache_spec:
- raise SandboxError("'remote-apis-socket' is not currently
supported with 'storage-service'.")
+ if config.remote_apis_socket_path and context.remote_cache_spec and
not self.re_remote:
+ raise SandboxError(
+ "'remote-apis-socket' is not supported with 'storage-service'
without 'action-cache-service'."
Review Comment:
So IIUC, we now support
* fully local: the buildbox-casd started by buildstream handles everything
(storage, execution, action cache)
* fully remote: the remote execution servers handle everything (cache
storage-service could double as storage for remote execution)
* local with remote action cache: execution is done locally, storage can be
either the `cache` storage-service or the `remote-execution` storage-service,
action cache has to be configured in `remote-execution`.
Assuming the above is correct, I find that this error message a bit
confusing as you can't set the `action-cache-service` in the `cache`
configuration.
##########
tests/integration/sandbox.py:
##########
@@ -59,3 +60,24 @@ def test_remote_apis_socket(cli, datafiles):
result = cli.run(project=project, args=["build", element_name])
assert result.exit_code == 0
+
+
+# Test configuration with remote action cache for nested REAPI.
[email protected](not HAVE_SANDBOX, reason="Only available with a
functioning sandbox")
[email protected](DATA_DIR)
+def test_remote_apis_socket_with_action_cache(cli, tmpdir, datafiles):
+ project = str(datafiles)
+ element_name = "sandbox/remote-apis-socket.bst"
+
+ with create_artifact_share(os.path.join(str(tmpdir), "remote")) as share:
+ cli.configure(
+ {
+ "remote-execution": {
+ "storage-service": {"url": share.repo},
+ "action-cache-service": {"url": share.repo, "push": True},
Review Comment:
Could you please add tests for the other possible configurations? (cache
storage-service with remote-execution action-cache-service, and both storage
services with remote-execution action-cache-service)
Slightly related, considering the case where both the `cache` and
`remote-execution` storage services are configured, does this lead to a
sensible outcome? (e.g. are blobs downloaded from the remote-execution storage
uploaded to the cache storage?)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
