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

Runtime parameters of mlcube can't include the letter "h" #347

Closed
hasan7n opened this issue Nov 27, 2023 · 3 comments
Closed

Runtime parameters of mlcube can't include the letter "h" #347

hasan7n opened this issue Nov 27, 2023 · 3 comments

Comments

@hasan7n
Copy link
Contributor

hasan7n commented Nov 27, 2023

Setting runtime parameters using the -P... convention doesn't work if the parameter contains the letter h. Example:
mlcube run -Pdocker.cpu_args="--shm-size 1g" ...
Running the above will show the MLCube's --help message.

Below as an example that can localize the problem:
mlcube run -Pdocker.cpu_args="--name helloworld" ...: This will show the --help message
mlcube run -Pdocker.cpu_args="--name elloworld" ...: This will run successfuly

The reason seems to be that when click encounters the following pattern: -**********h******, it interprets this as a bunch of options, and recognizes one of them: -h.

I couldn't find a workaround, and I think in the future we will need to allow users to dynamically set docker's --shm-size parameter.

@sergey-serebryakov
Copy link
Contributor

Thanks @hasan7n for reporting this. Will look at it on Friday.

sergey-serebryakov added a commit to sergey-serebryakov/mlbox that referenced this issue Dec 11, 2023
This commit fixes bugs associated with the MLCube's `run` command:
- It now uses correct option decorators from the `cli` (class Options) instead of deprecated decorators from the `__main__.py` file.
- It now uses the `-P` option to correctly parse MLCube parameters that users provide on a command line.

The latter particularly fixes the mlcommons#347 issue.
sergey-serebryakov added a commit that referenced this issue Dec 11, 2023
This commit fixes bugs associated with the MLCube `run` command:
- It now uses correct option decorators from the `cli` module (class `Options`) instead of deprecated decorators from the `__main__.py` file.
- It now uses the `-P` option to correctly parse MLCube parameters that users provide on a command line.

The latter particularly fixes the #347 issue.
@sergey-serebryakov
Copy link
Contributor

@hasan7n Ok, so it should be fixed in #348.

Brief description of the cause of the behavior. We've gone through multiple rounds of refactoring of the MLCube command line interface, and at some point in time option descriptions moved to their module (cli) while deprecated option declarations remained in the __main__.py file. The secondary cause of this bug was that we updated APIs of most of MLCube CLI commands, but did not do that for the run command that continued to use the old interface. After updating its API to match APIs of other commands (such as configure), this bug went away.
The main cause however was the following. The run command did not expose any arguments for the MLCube parameters (e.g., -Pdocker.cpu_args="--shm-size 1g"). Turns out that the click library makes various attempts to parse options/arguments and match them with the long/short aliases of the arguments. The following was hapenning:

  • The -Pdocker.cpu_args="--shm-size 1g" string is correctly split into name (-Pdocker.cpu_args) and value (--shm-size 1g).
  • The click library was trying to match the name against known aliases (long names) of options, and was obviously failing.
  • Then, it was trying to do fuzzy matching what also was resulting in empty matches.
  • At this point in time (I have not spent too much time identifying the exact logic) the click library considers the entire string -Pdocker.cpu_args="--shm-size 1g as multiple short options, that seems to be consistent with other tools, e.g, one can run ls -al which is the same as ls -l -a. So, the click library removes the first character (-) and then iterates one character at a time considering options such as -P, -d, -o etc. and eventually -h. All options happen to be unknown except the latter, which is the cause for click to display help message.

As you correctly mention, when there's no h, this bug does not manifest itself.

@dfeddema
Copy link
Contributor

Associated PR #348 fixed this problem.

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

No branches or pull requests

3 participants