address comments for containerize command#884
address comments for containerize command#884parano merged 7 commits intobentoml:masterfrom jackyzha0:update-containerize
Conversation
jackyzha0
commented
Jul 11, 2020
jackyzha0
commented
- rename _echo -> echo
- refactor to use load_bento_service_metadata to support file paths
- callback tag validation to cleanup containerize
- parameterized testing
|
Hello @jackyzha0, Thanks for updating this PR. There are currently no PEP 8 issues detected in this PR. Cheers! 🍻 Comment last updated at 2020-07-14 05:23:15 UTC |
Codecov Report
@@ Coverage Diff @@
## master #884 +/- ##
==========================================
- Coverage 61.82% 61.69% -0.13%
==========================================
Files 120 120
Lines 7918 7780 -138
==========================================
- Hits 4895 4800 -95
+ Misses 3023 2980 -43
Continue to review full report at Codecov.
|
bentoml/cli/bento_service.py
Outdated
There was a problem hiding this comment.
We should probably not do this either, "latest" is the default for docker, it is also a bad decision on docker side. And I think the behavior we want, is to use the BentoService version as the default if the user did not specify a version right?
bentoml/cli/bento_service.py
Outdated
There was a problem hiding this comment.
let's separate this to two separate functions? to_valid_docker_image_name and to_valid_docker_image_tag? Since the logic itself has nothing to do with BentoML
bentoml/cli/bento_service.py
Outdated
There was a problem hiding this comment.
bento here can be a local file path, you will need to update this message
bentoml/cli/bento_service.py
Outdated
There was a problem hiding this comment.
This behavior might be confusing to the end-user, I'd prefer to avoid implicitly converting the command argument they provide. For example, I write the following bash script to automate my deployment process:
saved_path="/Users/chaoyu/bentoml/repository/IrisClassifier/20200701222443_EC5EAB"
image_name="my-image-something-that-is-very-long..........."
bentoml containerize $saved_path -t $image_name
docker run $image_nameI will get an Unable to find image error due to the fact that the image name was modified implicitly.
I'm inclined to go with a simpler approach and only do the conversion if the value is automatically set from the BentoService metadata:
...
bs_metadata = load_bento_service_metadata(bento_service_bundle_path)
_echo(f"Building docker image for BentoService {bs_metadata.name}:{bs_metadata.version}")
if not tag:
tag = "{to_valid_docker_image_name(bs_metadata.name)}:{to_valid_docker_image_tag(bs_metadata.version)}"
if ":" not in tag: # user have specified image name but not image tag in -t
version= to_valid_docker_image_tag(bs_metadata.version)
tag = f"{tag}:{version}"
_echo("Building docker image with tag: {tag}")
docker_api = docker.APIClient()
...
| else: | ||
| name, version = tag, "" | ||
| if to_valid_docker_image_name(name) != name: | ||
| raise click.BadParameter( |
bentoml/cli/bento_service.py
Outdated
There was a problem hiding this comment.
Could we use a regex to actually validate the input? This can break if the user provides an upper case character or a non-ASCII character. see https://docs.docker.com/engine/reference/commandline/tag/#extended-description
Name components may contain lowercase letters, digits and separators. A separator is defined as a period, one or two underscores, or one or more dashes. A name component may not start or end with a separator.
A tag name must be valid ASCII and may contain lowercase and uppercase letters, digits, underscores, periods, and dashes. A tag name may not start with a period or a dash and may contain a maximum of 128 characters.
If it's too tricky to get this validation to match docker's rules, we can also just try to catch the invalid argument error when running docker build command, and show a proper error message to the user. Currently it shows a confusing Broken pip error when using an invalid docker tag:
> bentoml containerize IrisClassifier:latest -t abc___def
[2020-07-12 18:06:02,231] INFO - Getting latest version IrisClassifier:20200701222443_EC5EAB
Found Bento: /Users/chaoyu/bentoml/repository/IrisClassifier/20200701222443_EC5EAB
Bento tag was changed to be Docker compatible.
"IrisClassifier:latest" -> "abc___def:latest"
Building Docker image: abc___def
Traceback (most recent call last):
File "/Users/chaoyu/opt/miniconda3/envs/btml-dev/lib/python3.7/site-packages/urllib3/connectionpool.py", line 600, in urlopen
chunked=chunked)
File "/Users/chaoyu/opt/miniconda3/envs/btml-dev/lib/python3.7/site-packages/urllib3/connectionpool.py", line 354, in _make_request
conn.request(method, url, **httplib_request_kw)
File "/Users/chaoyu/opt/miniconda3/envs/btml-dev/lib/python3.7/http/client.py", line 1252, in request
self._send_request(method, url, body, headers, encode_chunked)
File "/Users/chaoyu/opt/miniconda3/envs/btml-dev/lib/python3.7/http/client.py", line 1298, in _send_request
self.endheaders(body, encode_chunked=encode_chunked)
File "/Users/chaoyu/opt/miniconda3/envs/btml-dev/lib/python3.7/http/client.py", line 1247, in endheaders
self._send_output(message_body, encode_chunked=encode_chunked)
File "/Users/chaoyu/opt/miniconda3/envs/btml-dev/lib/python3.7/http/client.py", line 1065, in _send_output
self.send(chunk)
File "/Users/chaoyu/opt/miniconda3/envs/btml-dev/lib/python3.7/http/client.py", line 987, in send
self.sock.sendall(data)
BrokenPipeError: [Errno 32] Broken pipe
| valid_name_pattern = re.compile( | ||
| r""" | ||
| ^( | ||
| [a-z0-9]+ # alphanumeric |
There was a problem hiding this comment.
love the regex inline comments here!
* address comments for containerize command * linting + naming fixes * containerize and tag validation refactor * fix tests * linting + formatting + more logging * add fix for non-ascii * regex based validation and updated tests
3 participants
