Skip to content

Conversation

qgallouedec
Copy link
Member

@qgallouedec qgallouedec commented Apr 9, 2025

From @lewtun

FYI I just install trl@main and got this from trying to run GRPO:

Traceback (most recent call last):
  File "/fsx/lewis/git/hf/dev/open-r1/openr1/bin/trl", line 4, in <module>
    from trl.cli import main
  File "/fsx/lewis/git/hf/dev/open-r1/openr1/lib/python3.11/site-packages/trl/cli.py", line 24, in <module>
    from .scripts.grpo import make_parser as make_grpo_parser
  File "/fsx/lewis/git/hf/dev/open-r1/openr1/lib/python3.11/site-packages/trl/scripts/grpo.py", line 22, in <module>
    from trl import GRPOConfig, GRPOTrainer, ModelConfig, ScriptArguments, TrlParser, get_peft_config
  File "<frozen importlib._bootstrap>", line 1229, in _handle_fromlist
  File "/fsx/lewis/git/hf/dev/open-r1/openr1/lib/python3.11/site-packages/trl/import_utils.py", line 127, in __getattr__
    value = getattr(module, name)
            ^^^^^^^^^^^^^^^^^^^^^
  File "/fsx/lewis/git/hf/dev/open-r1/openr1/lib/python3.11/site-packages/trl/import_utils.py", line 126, in __getattr__
    module = self._get_module(self._class_to_module[name])
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/fsx/lewis/git/hf/dev/open-r1/openr1/lib/python3.11/site-packages/trl/import_utils.py", line 138, in _get_module
    raise RuntimeError(
RuntimeError: Failed to import trl.trainer.grpo_trainer because of the following error (look up to see its traceback):
cannot import name 'LigerFusedLinearGRPOLoss' from 'liger_kernel.chunked_loss' (/fsx/lewis/git/hf/dev/open-r1/openr1/lib/python3.11/site-packages/liger_kernel/chunked_loss/__init__.py)

The solution is to bump liger-kernel like here but it's odd that doing a new install of trl didn't catch this (I had an older version of liger installed)

From me:

I think it's because you did pip install trl and not pip install trl[liger]. Maybe the solution to avoid this is to also check the version in is_liger_available (returns True only if the min version is installed) , and more generally in all is_xxx_availalbe.


I suggest the following solution. Now when you run is_liger_kernel_available without the min version, it returns False.

  1. It's a shame that you have to specify the min version in two places (setup.py and import_utils.py), but I can't think of any other easy and readable way to do it.

  2. This implementation actually matches that of transformers, which, as everyone knows, is the source of all truth 🙇: https://github.com/huggingface/transformers/blob/main/src/transformers/utils/import_utils.py

  3. We could do this with all our optional dependencies. But it's a bit time-consuming, so I'll leave it to follow-up PRs.

@qgallouedec qgallouedec changed the title is_liger_kernel_available with min version 🐯 is_liger_kernel_available with min version Apr 9, 2025
@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.

Copy link
Member

@lewtun lewtun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, LGTM!

@qgallouedec qgallouedec merged commit 982ba08 into main Apr 9, 2025
9 of 10 checks passed
@qgallouedec qgallouedec deleted the is_liger_kernel_available branch April 9, 2025 13:44
yxliu-TAMU pushed a commit to mincheolseong/ECEN743-GRPO-Project-Proposal that referenced this pull request Apr 20, 2025
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

Successfully merging this pull request may close these issues.

4 participants