search

gabrieldemarmiesse / python-on-whales

An awesome Python wrapper for an awesome Docker CLI!
MIT License
561 stars 102 forks source link

Inconsistent representation of cpusets #626

Open LewisGaul opened 2 months ago

LewisGaul commented 2 months ago

ContainerCLI.create() and ContainerCLI.run() support cpuset_cpus and cpuset_mems as List[int], which is converted to a CLI arg using ",".join(...).

ContainerCLI.update() supports cpuset_cpus and cpuset_mems as List[int] in the type signature but then converts to a CLI arg simply by stringifying (implicitly in utils.run()), which I assume will always fail.

In reality the CLI accepts a cpuset representation, which includes: a range, e.g. "0-3", a selection, e.g. "0,2", a combination of both, e.g. "0-3,7".

I'd suggest it'd make most sense just to support the user specifying this as a string, rather than only allowing the "selection" format via a list. This would mean fixing the type annotation on update() and a backwards incompatible change on create() and run().

LewisGaul commented 2 months ago

@gabrieldemarmiesse how should we handle this backwards incompatible change, assuming you're happy with what I'm proposing? Should the create() and run() methods still accept List[int] and emit a deprecation warning? Or is this something we can just change?

gabrieldemarmiesse commented 1 month ago

@LewisGaul do you think it would make sense to have List[int] | str? With List[int] being there when someone is programmatically choosing the cpuset. This option is more powerful than the range and combinaison, and is intuitive.

Let me know if I'm missing something, it's not an option I use often.

LewisGaul commented 1 month ago

@LewisGaul do you think it would make sense to have List[int] | str? With List[int] being there when someone is programmatically choosing the cpuset. This option is more powerful than the range and combinaison, and is intuitive.

Seems reasonable to me