Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed decoding step and activate decoding by client #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EmilEOGG
Copy link
Contributor

@EmilEOGG EmilEOGG commented Jul 6, 2022

FIX

There was an issue when decoding the docker logs with the json package on @bibs2091 's machine. Removing the two-step decoding and adding the decode argument to the client.build() call seems to fix the issue.

Additional context

@bibs2091 please add screenshots and error logs as well as your hardware, OS and software versions for everything.

@EmilEOGG EmilEOGG requested a review from jgsch July 6, 2022 13:35
@bibs2091
Copy link

bibs2091 commented Jul 6, 2022

environment:

  • OS: macOS Monterey 12.4
  • Chip: apple M1
  • python: 3.9.12
  • docker: 20.10.17
  • docker api: 1.41
  • docker desktop: 4.10.0
  • i2-client: 0.4.0 (installed in conda environment)

Error Message

Tried i2 on the mirror image:
image

Tracing the error put me in this part of code:
image
I printed output before the json line and it gave this:
b'{"stream":"Step 1/2 : FROM alpineintuition/archipel-base-gpu"}\r\n{"stream":"\\n"}\r\n'

@bibs2091
Copy link

bibs2091 commented Jul 6, 2022

I had windows as an os and the i2 didn't work for me in the first place (refer to this https://github.com/alpineintuition/i2-cli/issues/30). So if it works on Linux and macOS is good enough!!

output = generator.__next__()
except docker.errors.APIError as error:
raise docker.errors.BuildError(reason=error.explanation, build_log=error)

output = json.loads(output.decode())

# Setup progress bar
num_steps = int(output["stream"].split()[1].split("/")[-1])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work because we need the output to determine the number of steps.

Case like this (b'{"stream":"\\n"}\r\n') happend lot of times during the build, just ignore with something like:

output = json.loads(output.decode().split("\n")[0])

Maybe add this to the other place where we decode the docker output if it happend at all step.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the pattern is not regular, sometimes there is a lot of /n before the content we want. but I will try ur solution nevertheless

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants