这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@yao-matrix
Copy link
Contributor

@yao-matrix yao-matrix commented Jun 24, 2025

@BenjaminBossan , pls help review, thx very much.

Signed-off-by: YAO Matrix <matrix.yao@intel.com>
Signed-off-by: YAO Matrix <matrix.yao@intel.com>
@yao-matrix yao-matrix marked this pull request as draft June 24, 2025 05:27
@yao-matrix yao-matrix marked this pull request as ready for review June 24, 2025 06:18
Copy link
Member

@BenjaminBossan BenjaminBossan 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 PR.

I'm unsure at the moment if we want to make the PEFT method comparison code device-agnostic. Our plan is to always run it on the same device to make it possible to compare the results in a fair manner. Even if users submit their own experiments, we would still run it ourselves. As CUDA is still the dominant platform, there is thus little need for the code to be device agnostic.

That said, I can see that the changes are quite small and there are some advantages for users who want to run this at home who don't have NVIDIA GPUs. @githubnemo WDYT?

@githubnemo
Copy link
Collaborator

I'm in favor of supporting XPU.

Regarding official benchmark numbers, if there was a device that we could run the benchmarks on, I would even go as far as maintaining a second set of benchmarks. But there doesn't seem to be a way, currently, so this feature will only benefit those people who want to run the benchmarks locally.

@BenjaminBossan
Copy link
Member

Okay, then let's do it!

Before proceeding with this PR: It will probably conflict with #2602, right? How would you suggest to proceed? First merge #2602 and then update this PR? All the results files might need updating, but it should hopefully just be some simple string replacements.

yao-matrix and others added 5 commits June 24, 2025 23:49
---------

Signed-off-by: YAO Matrix <matrix.yao@intel.com>
FIX Transformers VLM architecture changes

Follow up to huggingface#2554
See discussion in huggingface/transformers#38627

To quote:

> transformers PR #37033 re-arranges the way visual language models are
built by moving the LM head from the language model to the top-level
VLM (among other things).

A consequence of this is that the keys in the PEFT state_dict now also
follow the new architecture. This means that:

1. If a PEFT checkpoint was saved with the old architecture but is
   loaded with the new architecture, loading fails.
2. If a PEFT checkpoint was saved with the new architecture but is
   loaded with the old architecture, loading fails.

1. can be addressed by making use of the newly added
_checkpoint_conversion_mapping attribute for models with the new
architecture. In transformers, this is used to map old model state_dicts
to the new state_dict format. In PEFT, with some fiddling, we can use
the same mapping to make old PEFT state_dicts compatible with the new
architecture (backwards compatibility).

However, 2. is not easily addressed. We would need a reverse mapping for
this. This could be easily derived from _checkpoint_conversion_mapping,
but since this attribute doesn't exist on old models, we cannot do that.
Therefore, new checkpoints created with PEFT on these models won't load
successfully when users use old transformers (forward compatibility).

These cases are covered by the added unit tests, which means that the
test covering case 2 are marked as xfail.

If we could reliably detect that we are in case 2, we could warn the
user and advise them to upgrade transformers, but I don't know if it's
possible to figure this out.

We also allow users to pass their own key_mapping to from_pretrained and
load_adapter, though the documentation advises against it. This argument
could theoretically be used as a workaround in case there is indeed an
issue with prompt learning state_dicts.

Apart from these changes, I also made a small change to account for
huggingface/transformers#38017 (comment).
---------

Signed-off-by: jiqing-feng <jiqing.feng@intel.com>
This change updates all results with their respective number of
parameters (trained + absolute) and adds the newly introduced
full-finetuning.

In addition to these results there was also an issue with the
Makefile as it didn't consider the possibility of having experiments
that don't have an adapter config (e.g., full fine-tuning).
Signed-off-by: YAO Matrix <matrix.yao@intel.com>
@yao-matrix yao-matrix closed this Jun 25, 2025
@yao-matrix yao-matrix deleted the method-comp-xpu branch June 25, 2025 00:46
cyyever pushed a commit to cyyever/peft that referenced this pull request Sep 4, 2025
* initial commit

* doc on custom reward function

* test

* doc doc doc

* fix collator

* style

* links?

* I need a docdoc 🎵

* fix link

* I do like writing doc tbh

* it takes time, but it's worth it

* no return!

* type hint

* it's probably the best of both worlds [ci skip]

* new doc before implementation

* tests

* more doc

* style

* multiple pretrained funcs

* fix arg name

* main?

* example for R1

* fix script

* clearer

* import [ci skip]

* Update docs/source/grpo_trainer.md

Co-authored-by: lewtun <lewis.c.tunstall@gmail.com>

---------

Co-authored-by: lewtun <lewis.c.tunstall@gmail.com>
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