feat: print callback for docker image build#481
feat: print callback for docker image build#481qwertycxz wants to merge 2 commits intoagentscope-ai:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a configurable output callback for Docker image build/push streaming output, replacing direct print() calls so callers can intercept and process docker build/docker push logs (Issue #480).
Changes:
- Add a
printcallback toImageConfigandBuildConfig, defaulting to the built-inprint. - Route streamed
docker buildoutput throughconfig.print(...)instead of callingprint(...)directly. - Update
push_imageto acceptbuild_config(instead of aquietboolean) and routedocker pushstreaming output through the configured callback.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/agentscope_runtime/engine/deployers/utils/docker_image_utils/image_factory.py |
Adds a configurable print callback into image build configuration passed to the Docker builder. |
src/agentscope_runtime/engine/deployers/utils/docker_image_utils/docker_image_builder.py |
Uses the callback for streaming output and refactors push_image to accept full build configuration. |
| no_cache: bool = False | ||
| quiet: bool = False | ||
| print: Callable[[str], None] = print | ||
| build_args: Dict[str, str] = {} |
There was a problem hiding this comment.
print as a model field name is likely to trigger pylint's redefined-builtin (W0622), which will fail pre-commit unless suppressed. Consider either renaming the field to something like output_callback (and using a Pydantic alias so callers can still pass print=...), or add a targeted # pylint:disable=redefined-builtin for this file/class attribute.
There was a problem hiding this comment.
| while True: | ||
| output = process.stdout.readline() | ||
| if output == "" and process.poll() is not None: | ||
| break | ||
| if output: | ||
| print(output.strip()) | ||
| config.print(output.strip()) | ||
|
|
There was a problem hiding this comment.
This new output callback path isn’t covered by tests: existing tests only exercise quiet=True (subprocess.run) and won’t catch regressions in streaming mode. Add a unit test for quiet=False that mocks subprocess.Popen stdout and asserts the configured callback is invoked with streamed lines.
There was a problem hiding this comment.
Not a part of my PR.
| while True: | ||
| output = process.stdout.readline() | ||
| if output == "" and process.poll() is not None: | ||
| break | ||
| if output: | ||
| print(output.strip()) | ||
| build_config.print(output.strip()) | ||
|
|
There was a problem hiding this comment.
Streaming output in push_image now goes through the callback, but there’s no test coverage for this path. Add a unit test that mocks subprocess.Popen for docker push and asserts build_config.print (or the configured callback) receives the streamed output lines when quiet=False.
There was a problem hiding this comment.
Not a part of my PR.
| # Build configuration | ||
| no_cache: bool = False | ||
| quiet: bool = False | ||
| print: Callable[[str], None] = print | ||
| build_args: Dict[str, str] = {} |
There was a problem hiding this comment.
print as a model field name is likely to trigger pylint's redefined-builtin (W0622), which will fail pre-commit unless suppressed. Consider renaming the field to something like output_callback (and using a Pydantic alias so callers can still pass print=...), or add a targeted # pylint:disable=redefined-builtin for this file/class attribute.
There was a problem hiding this comment.
Description
Make
printnot a bare function, but a callback function which we could determine inBuildConfig.我想就不要用一个裸的
print了，可以将其修改成一个回调函数，允许我们在BuildConfig中指定。Related Issue: Fix #480
Security Considerations: None, I suppose
Type of Change
Component(s) Affected
Checklist
Testing
Add a
print=fooparam inapp.deploy().在
app.deploy()中加入print参数即可测试。Additional Notes
Unless explicitly set, there is no change in behavior.
除非显式设置，否则行为上没有改变。