Help for `mlem declare deployment docker_container` have a unusable option

[aguschin]

I see two arguments in --help:

│ --port_mapping                      DICT  Expose ports [default: __NOT_SET__]                                                                        │
│ --port_mapping.key                  INT   Expose ports [default: __NOT_SET__]                                                                        │

the example I posted is about using the 2nd option. What about the 1st option? Wasn't able to use it somehow. Does it works? If yes, could you please post an example? If not, let's simply remove it?

Originally posted by @aguschin in https://github.com/iterative/mlem/pull/479#pullrequestreview-1175226800

@mike0sv's answer

This is not new actually. I think I left it there to highlight that this field is dict. But that's another issue

[aguschin]

This confused me a bit. From one hand, telling it's a dict is a hint on how to use the 2nd option. From the other hand, it feels like an extra option that doesn't need to exist.

Should we explain the same in Expose ports help string? Like Expose ports in port_mapping.key=value form?

[mike0sv]

No, help string is from class field docstring, where CLI stuff will not make sense. And it's --port_mapping.key value, so that might be enough?

[omesser]

@iterative/cml - Is it just me or is this a super weird way to declare port mapping in a CLI (a 1-key dict)? the format is very alien to people who used something similar in docker.

Why not go the more "standard/expected" form of --port_mapping being of type string and the structure supporting --port_mapping 8000 (=8000:8000), or --port_mapping 8000:9000 ? the help string would explain it's "[container:]host" mapping If we're supporting multiple ports then it could be extended to a list with indices --port_mappings.0 [container:]host wdyt? 🤔

[mike0sv]

Mostly because we have very complicated logic to turn class fields into cli options and then parse them back into those classes :) And it's all dynamically generated, so no room for exceptions.

We could, however, make the original field a list of strings so you can do something like --port_mappings.0 [container:]host, but that will look weird for API usage probably

[mike0sv]

Oh, that's probably exactly what you meant :)

[0x2b3bfa0]

Does the current syntax expect users to specify --port_mapping.80=8080 to achieve the same effect as e.g. --publish 8080:80 in a docker run invocation? 🤯

https://github.com/iterative/mlem/blob/67002975dced789290dc933bcec45256d3f938e9/tests/contrib/test_docker/test_deploy.py#L129

docker documentation

Published ports

By default, when you create or run a container using docker create or docker run it does not publish any of its ports to the outside world. To make a port available to services outside of Docker, or to Docker containers which are not connected to the container’s network, use the --publish or -p flag. This creates a firewall rule which maps a container port to a port on the Docker host to the outside world. Here are some examples.

Flag value	Description
-p 8080:80	Map TCP port 80 in the container to port 8080 on the Docker host.
-p 192.168.1.100:8080:80	Map TCP port 80 in the container to port 8080 on the Docker host for connections to host IP 192.168.1.100.
-p 8080:80/udp	Map UDP port 80 in the container to port 8080 on the Docker host.
-p 8080:80/tcp -p 8080:80/udp	Map TCP port 80 in the container to TCP port 8080 on the Docker host, and map UDP port 80 in the container to UDP port 8080 on the Docker host.

docker-py documentation

Ports

Ports to bind inside the container.

The keys of the dictionary are the ports to bind inside the container, either as an integer or a string in the form port/protocol where the protocol is either tcp, udp, or sctp

The values of the dictionary are the corresponding ports to open on the host, which can be either:

The port number, as an integer. For example, {'2222/tcp': 3333} will expose port 2222 inside the container as port 3333 on the host

None, to assign a random host port. For example, {'2222/tcp': None}

A tuple of (address, port) if you want to specify the host interface. For example, {'1111/tcp': ('127.0.0.1', 1111)}

A list of integers, if you want to bind multiple host ports to a single container port. For example, {'1111/tcp': [1234, 4567]}

[0x2b3bfa0]

Is it just me or is this a super weird way to declare port mapping in a CLI (a 1-key dict)? the format is very alien to people who used something similar in docker.

I think the same: this syntax deviates considerably from what I would expect, and it took me some code reading to figure out the details; users may not be as patient as we are.

[mike0sv]

Ok, I will change the logic to match docker cli syntax and parse it into what docker-py expects. Do I understand correctly that {'1111/tcp': [1234, 4567]} for docker-py actually means -p 1234:1111/tcp -p 4567/1111/tcp for cli?

[mike0sv]

Created a PR. Please suggest more tests cases I might missed. Note that this issue originally not about that, but about cli help for all dict/list arguments, eg for mlem declare builder docker -h that has

│    --args.templates_dir                LIST  directory or list of directories for Dockerfile templates - `pre_install.j2` - Dockerfile commands to run before pip - `post_install.j2` - Dockerfile commands  │
│                                              to run after pip - `post_copy.j2` - Dockerfile commands to run after pip and MLEM distribution copy                                                             │
│                                              [default: __NOT_SET__]                                                                                                                                          │
│    --args.templates_dir.0              STR   directory or list of directories for Dockerfile templates - `pre_install.j2` - Dockerfile commands to run before pip - `post_install.j2` - Dockerfile commands  │
│                                              to run after pip - `post_copy.j2` - Dockerfile commands to run after pip and MLEM distribution copy                                                             │
│                                              [default: __NOT_SET__]                                                                                                                                          │