🕹️ CLI refactor

[qgallouedec]

What does this PR do?

Historically

The example scripts were initially placed outside the package directory (.examples/scripts) because they were not intended for runtime use. However, with the introduction of the TRL CLI in #1419, these scripts became executable at runtime through the CLI. Unfortunately, the scripts could not be directly imported from the package. To address this, we implemented a workaround in setuptools by creating a symbolic link from trl/trl/commands to examples/scripts.

Motivations for this refactor

• Recurrent issues with the simlink not properly created: trl CLI doesn't work #1716, Not find sft.py #1829 Fix dev install #2369 (comment)

• Populating the CLI with new command isn't flexible [CLI] Extend training support to all trainers #2101 populate SUPPORTED_COMMANDS cli #2157

• We implement our own parser based on sys.argv to have our own subparsing logic, while argparse have everything we need for subparsing

• We don't have the usual parsing associated features like the help etc:

  $ trl dpo --help
  The following values were not passed to `accelerate launch` and had defaults used instead:
          `--num_processes` was set to a value of `1`
          `--num_machines` was set to a value of `1`
          `--mixed_precision` was set to a value of `'no'`
          `--dynamo_backend` was set to a value of `'no'`
  To avoid this warning pass in values for each of the problematic parameters or run `accelerate config`.
  /fsx/qgallouedec/miniconda3/envs/trl/bin/python: can't open file '/fsx/qgallouedec/trl/trl/commands/scripts/dpo.py': [Errno 2] No such file or directory
  Traceback (most recent call last):
    File "/fsx/qgallouedec/miniconda3/envs/trl/bin/accelerate", line 8, in <module>
      sys.exit(main())
              ^^^^^^
    File "/fsx/qgallouedec/miniconda3/envs/trl/lib/python3.11/site-packages/accelerate/commands/accelerate_cli.py", line 48, in main
      args.func(args)
    File "/fsx/qgallouedec/miniconda3/envs/trl/lib/python3.11/site-packages/accelerate/commands/launch.py", line 1168, in launch_command
      simple_launcher(args)
    File "/fsx/qgallouedec/miniconda3/envs/trl/lib/python3.11/site-packages/accelerate/commands/launch.py", line 763, in simple_launcher
      raise subprocess.CalledProcessError(returncode=process.returncode, cmd=cmd)
  subprocess.CalledProcessError: Command '['/fsx/qgallouedec/miniconda3/envs/trl/bin/python', '/fsx/qgallouedec/trl/trl/commands/scripts/dpo.py', '--help']' returned non-zero exit status 2.
  [10:18:19] TRL - DPO failed on ! See the logs above for further details.                                    cli.py:98
  Traceback (most recent call last):
    File "/fsx/qgallouedec/trl/trl/commands/cli.py", line 89, in train
      subprocess.run(
    File "/fsx/qgallouedec/miniconda3/envs/trl/lib/python3.11/subprocess.py", line 571, in run
      raise CalledProcessError(retcode, process.args,
  subprocess.CalledProcessError: Command '['accelerate', 'launch', '/fsx/qgallouedec/trl/trl/commands/scripts/dpo.py', '--help']' returned non-zero exit status 1.
  
  The above exception was the direct cause of the following exception:
  
  Traceback (most recent call last):
    File "/fsx/qgallouedec/miniconda3/envs/trl/bin/trl", line 8, in <module>
      sys.exit(main())
              ^^^^^^
    File "/fsx/qgallouedec/trl/trl/commands/cli.py", line 131, in main
      train(command_name)
    File "/fsx/qgallouedec/trl/trl/commands/cli.py", line 99, in train
      raise ValueError("TRL CLI failed! Check the traceback above..") from exc
  ValueError: TRL CLI failed! Check the traceback above..
  

• Building the package will soon not be possible because of the simlink to the examples, see Fix dev install #2369 (review)

• Recurrent and enigmatic CI failures: Operation was cancelled example here (I'm not sure this will solve the problem, we'll see)

Alternatives

Solution

Reference structure for the new CLI: https://github.com/qgallouedec/turbo-spoon

Project structure

From                                                     To
.                                                        .
├── examples                                             ├── setup.py           
│   └── scripts                                          └── trl              
│       └── dpo.py                                           ├── __init__.py                 
├── setup.py                                                 ├── cli.py           
└── trl                                                      └── scripts      
    ├── __init__.py                                              ├── __init__.py                  
    └── commands                                                 └── dpo.py               
        ├── __init__.py
        ├── cli.py
        └── scripts (simlink to examples/scripts)

Setup file

try:
    file_path = os.path.dirname(os.path.abspath(__file__))
    os.symlink(os.path.join(file_path, "examples/scripts"), os.path.join(file_path, "trl/commands/scripts"))

    setup(
        name="trl",
        entry_points={"console_scripts": ["trl=trl.commands.cli:main"]},
    )
finally:
    os.unlink(os.path.join(file_path, "trl/commands/scripts"))

to

setup(
    name="trl",
    entry_points={"console_scripts": ["trl=trl.cli:main"]},
)

Why this solution?

From the setuptools user guide

Please note that, when using include_package_data=True, only files inside the package directory are included in the final wheel, by default.

So for example, if you create a Python project that uses setuptools-scm and have a tests directory outside of the package folder, the tests directory will be present in the sdist but not in the wheel [2].

See Data Files Support for more information.

---

[[2]] This happens because the sdist can contain files that are useful during development or the build process itself, but not in runtime (e.g. tests, docs, examples, etc…). The wheel, on the other hand, is a file format that has been optimized and is ready to be unpacked into a running installation of Python or Virtual Environment. Therefore it only contains items that are required during runtime.

Consequently, if we want to have access to the example with the CLI (at runtime then)

Examples and scripts

What we've called examples are now actual scripts.

Criterion	Script	Example
Purpose	Solve a problem or execute a task.	Demonstrate a concept or functionality.
Autonomy	Self-contained, executable as-is.	Not standalone, intended for integration or testing.
Complexity	Can be complex and structured.	Usually simple and focused.
Primary Use	Automation, practical tasks.	Learning, documentation, demonstration.

But these scripts are still relevant as examples, and are widely used as examples not only in our documentation, but also externally. That's why we need to ensure that these scripts are still referenced in order to avoid creating dead link:

###############################################################################################
# This file has been moved to https://github.com/huggingface/trl/blob/main/trl/scripts/dpo.py #
###############################################################################################

This PR will only handle the commands currently supported: sft, dpo and chat. We will deal with the other scripts one by one in subsequent PRs.

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?

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.

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[lewtun]

If I understand correctly, we will need to move all example scripts from examples/scripts to trl/scripts right? I am wondering if this causes some downstream issues if people have linked their websites to examples/scripts and whether it will be harder for people to find the scripts now since it's somewhat uncommon to include such objects in the package directly.

I agree that in general we want to have a robust CLI that isn't afflicted by all the symlink issues you've noted, but let's also see what @lvwerra thinks since him and Younes were the architects of that feature

[qgallouedec]

Yes, that's the main question. Sorry I didn't have time to go into detail about my thoughts in the original post (still under construction).

downstream issues if people have linked their websites

Right. The links wouldn't be dead in all cases, as files like https://github.com/huggingface/trl/blob/adac644f7fe1a5a37f9cca10044ed81b594eae32/examples/scripts/dpo.py would allow redirection.

As far as this point is concerned, I think refraining to move these files is a case of the snake biting its own tail, because the number of link will grow.

harder for people to find the scripts

Probably slightly harder yes.

I'd say these are the two main drawbacks (link issues and harder to find) to weigh in the balance.

uncommon to include such objects in the package directly

True, examples aren't included in the package. But I think here, it's more scripts that can be used as examples, rather than true examples. I think this mostly because:

1. we have full argparsing support,
2. they are used in the CLI.

[qgallouedec]

I had a short discussion with @lvwerra (I don't want to change what you said, feel free to correct me).

• The main hurdle with the refactoring is that it risks breaking links.
• The idea of not deleting example files so that links aren't dead is a good one.
• In some cases, we could open PRs on repos to update examples links.
• Having a duplicate with examples (in examples/scripts ) and scripts (in trl/scripts) would make maintenance more arduous, which is why this solution was not chosen at first.

As a first iteration, we could only move dpo, sft, and chat (current cli support).
This new separation makes even more sense to me considering we're starting to add true example (ie, not script meant to be used at runtime) like in #2409 #1518 #2336 #1647