Docker build #815
Docker build #815
Conversation
facebook-github-bot
added
the
CLA Signed
I also removed a redundant error case in my commit ( next() doesnt raise ValueError) and renamed output to build_msg. |
|
My previous commit #809 was flawed, it didn't change logic cause if it did, it would introduce a bug. Ill fix it in a different commit, in the meantime this commit is in draft mode. |
|
@ccharest93 are you interested in polishing this up and landing it? cc @clumsy |
| if len(old_imgs) == 0 or role.image not in old_imgs: | ||
| role.image = image.id | ||
| build_msg: Dict[str, str] = {} | ||
| while True: |
There was a problem hiding this comment.
Can't we use a simple for loop?
for build_msg in build_events:
if "stream" in build_msg:
output_str = build_msg["stream"].strip("\r\n").strip("\n")
if output_str:
log.info(output_str)
if "aux" in build_msg:
image_id = build_msg["aux"]["ID"]
breakThere was a problem hiding this comment.
build events is a stream, until stop_iteration is given we need to wait for further input on the stream. A for_loop wouldn't wait for the stream to complete and would only print events up to this point
There was a problem hiding this comment.
Isn't it how Iterables/generators work in for loop in Python? If you give a for loop an Iterable/generator (something that has __next__()) it will call __next__() until it faces StopIteration and exists the loop.
There was a problem hiding this comment.
Seem to be working fine, please check the latest version of #874
| @@ -119,7 +131,7 @@ def build_workspace_and_update_role( | |||
| f"failed to pull image {role.image}, falling back to local: {e}" | |||
| ) | |||
| log.info("Building workspace docker image (this may take a while)...") | |||
| image, _ = self._docker_client.images.build( | |||
| build_events = self._docker_api_client.build( | |||
There was a problem hiding this comment.
Why can't we simply use the second value in the tuple that self._docker_client.images.build() returns? It's the same build_events Iterable, isn't it? Then we don't have to copy the logic to retrieve the image id.
There was a problem hiding this comment.
Is it done so that we can print all the output leading to the error?
There was a problem hiding this comment.
If so we don't seem to print error events
There was a problem hiding this comment.
if i recall correctly, the first call is blocking and returns all the events at the same time, the second isnt and returns a stream which we can then print/process in real time
|
Ok i cleaned up the commit, i still need to rebuild my devcontainer to run tests and fix any errors that show up, will do this weekend. Would still like feedback on my original post though. Don't want to end up cluttering the torchx logs too much is my opinion.
|
|
@ccharest93 adding |
Added a docker API client, and changed the build function to its low-level version. This makes it return an event stream that we can then log to screen for real-time update on the image build process. This prints the same things as running docker build locally, so docker RUN commands can get quite verbose, we could filter the 'stream' events and only print the ones starting with 'Step ' or '---> ' .
Also changed the log message from api.py to make more sense (encompasses the case where dockerfile.torchx is used).
#813