populate SUPPORTED_COMMANDS cli

[grumpyp]

What does this PR do?

This closed #2101

Fixes # (issue)

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline,
  Pull Request section?
• Was this discussed/approved via a GitHub issue? Please add a link
  to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines.
• Did you write any new necessary tests?

Who can review?

@lewtun
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

[qgallouedec]

Thanks for contributing. I would suggest a simpler approach. Just modify

trl/trl/commands/cli.py

Lines 130 to 142 in 78249d9

	def main():
	command_name = sys.argv[1]
	if command_name in ["sft", "dpo", "kto"]:
	train(command_name)
	elif command_name == "chat":
	chat()
	elif command_name == "env":
	print_env()
	else:
	raise ValueError(
	f"Please use one of the supported commands, got {command_name} - supported commands are {SUPPORTED_COMMANDS}"
	)

into

 def main(): 
     command_name = sys.argv[1] 

     if command_name == "chat": 
         chat() 
     elif command_name == "env": 
         print_env() 
    else:
        train(command_name)

It should be enough, what do you think?

[grumpyp]

#2101
Hi @qgallouedec

Thanks for the feedback! I see your point about simplifying the approach. My initial thought was to keep the existing structure to maintain clarity for users regarding supported commands.

I mean it could be done in that straight forward way but as a user I would not know what commands I could use.

To find out I'd actually have to dig into the code and see what's gonna be executed e.g. how train works.

Maybe I am overcomplicating things here. This is my first contribution so I don't know what kinda users (weather technical enough or not) are using trl.

Either way, I will adjust the implementation based on what you think is best!

[qgallouedec]

As a user I'd use

trl --help

We're currently tweaking sys.argv instead of a proper argparse. That's why the above command won't give anything.
But in the future, I'd like to use argparse instead.

[grumpyp]

@lewtun anything to say here maybe? You opened the issue and might have some additional suggestion.

trl --help wouldn't work and is currently also not working

Either way, I am happy to go with your suggestion. Please let me know how you'd want it.

[qgallouedec]

trl --help wouldn't work and is currently also not working

In the future, we will probably move to subparser for the trl cli, so trl --help will output something.

Either way, I am happy to go with your suggestion. Please let me know how you'd want it.

Do you mind trying with the above suggestion? Also, the critical point here is to add the tests: like this one

trl/tests/test_cli.py

Lines 20 to 28 in 78249d9

	def test_sft_cli():
	try:
	subprocess.run(
	"trl sft --max_steps 1 --output_dir tmp-sft --model_name_or_path trl-internal-testing/tiny-random-LlamaForCausalLM --dataset_name stanfordnlp/imdb --learning_rate 1e-4 --lr_scheduler_type cosine --dataset_text_field text",
	shell=True,
	check=True,
	)
	except BaseException as exc:
	raise AssertionError("An error occured while running the CLI, please double check") from exc

[grumpyp]

trl --help wouldn't work and is currently also not working

In the future, we will probably move to subparser for the trl cli, so trl --help will output something.

Either way, I am happy to go with your suggestion. Please let me know how you'd want it.

Do you mind trying with the above suggestion? Also, the critical point here is to add the tests: like this one

trl/tests/test_cli.py

Lines 20 to 28 in 78249d9

	def test_sft_cli():
	try:
	subprocess.run(
	"trl sft --max_steps 1 --output_dir tmp-sft --model_name_or_path trl-internal-testing/tiny-random-LlamaForCausalLM --dataset_name stanfordnlp/imdb --learning_rate 1e-4 --lr_scheduler_type cosine --dataset_text_field text",
	shell=True,
	check=True,
	)
	except BaseException as exc:
	raise AssertionError("An error occured while running the CLI, please double check") from exc

Hi,

do you want me to add a test for each model or some dynamic way?

So if I understand correctly, you want to use the test you just proposed and this approach:

 def main(): 
     command_name = sys.argv[1] 

     if command_name == "chat": 
         chat() 
     elif command_name == "env": 
         print_env() 
    else:
        train(command_name)

?

[qgallouedec]

do you want me to add a test for each model or some dynamic way?

For each model. The args may vary a lot so I don't think it's possible to have a generic test for all scripts

So if I understand correctly, you want to use the test you just proposed and this approach:

That's right