Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. When it is ready for review, please click the Ready for review button (at the bottom of the PR page).

For me it seems the problem is in how qwen2-vl is written. Other VLMs do not override the text and use save expanded prompts in a new list. So rewriting Qwen2-VL is more preferable than adding a copy() in all processors

@zucchini-nlp - thanks for the comment! 😃

Just to be clear: I added the copy() in all the processors that have a similar pattern like:

for sample in text:
    sample = sample.replace(...)

This isn't just Qwen-based processors. One example: Pixtral.

So, if I understand correctly, you're saying it'd be preferrable to change all these processors to have something like:

expanded_samples = []
for sample in text:
    expanded_sample =  sample.replace(...)
    expanded_samples.append(expanded_sample)

Correct?

I must say: I don't find that cleaner 🤔 and if it's for the sake of consistency then I'd suggest adding the .copy() to the rest of the VLM processors. That would imply less changed lines and better readability IMO -- e.g. no variable name change. However, if what you're saying is the consensus then I'd be happy to make the changes.

That would imply less changed lines

ha! 🤔 don't think that's true. My bad.

Still, it seems a bit less readable to me.

Again, happy to go either way if that's your suggestion.

@nph4rd not a big difference for me, I am good whichever way as long as it doesn't do in-place modifications. I was surprised to see so many processor's changed, while the error is applicable only to Qwen VL

Let's also see what @molbap thinks

@molbap any thoughts here? This is a pretty tricky bug when encountered downstream that is hurting the larger ecosystem

Sorry, qwen has been fixed already as part of a different PR. We might not need this fix anymore, other VLMs don't do in-place modification

@qubvel - apologies for the delay. I've just merged main.

Missed out on this - I think we should not mutate inputs indeed.

I think it was already fixed as part of another PR already for qwen models, and other models don't mutate input

I think it was already fixed as part of another PR already for qwen models, and other models don't mutate input

@zucchini-nlp - could you link the PR? I can't find it

I'm not sure about the other models. I don't see why they wouldn't. I tried to include all of the processors that had in-place mutations.

@nph4rd it was #37342 because some tests would fail otherwise (I think that test was modified right before merging though). And since the test passed for other models, I assume no other processor has in-place modifications. We usually save expanded inputs in a new list, and qwen was an more of an exception 😄

If you see other models doing in-place modification, feel free to rebase and update the PR. Will be very much welcome. And adding a test would be nice so we catch these processors before merging

Ah, I see, @zucchini-nlp. Looks good! Thanks for the heads up.

In that case, @molbap, I'll close this PR and the corresponding issue 👍