Skip to content

Conversation

@booxter
Copy link
Contributor

@booxter booxter commented Apr 7, 2025

Since huggingface/transformers#36794 it handles
the return_dict with can_return_tuple decorator that expects the
argument to be passed as kwarg.

Without the patch, instructlab functional tests fail with:

AttributeError: 'bool' object has no attribute 'unsqueeze'

This is because the passed return_dict argument is incorrectly
interpreted as cache_position.

Of course, the way the model forward method is overridden here is quite
problematic and would benefit from a refactor. This patch is a stop-gap
to fix the CI with minimal changes.

Signed-off-by: Ihar Hrachyshka [email protected]

@booxter
Copy link
Contributor Author

booxter commented Apr 7, 2025

@JamesKunstle please take a look. I am not sure about the change since I don't grasp the full extent of the override; I think I understand the root cause and I did my best to clean the code up from mere mechanical code reading. Someone with proper expertise would need to check if this makes in a broader context of what the code is trying to achieve beyond... not crashing.

@booxter booxter marked this pull request as draft April 7, 2025 23:25
Since huggingface/transformers#36794 it handles
the return_dict with can_return_tuple decorator that expects the
argument to be passed as kwarg.

Without the patch, instructlab functional tests fail with:

AttributeError: 'bool' object has no attribute 'unsqueeze'

This is because the passed return_dict argument is incorrectly
interpreted as cache_position.

Of course, the way the model forward method is overridden here is quite
problematic and would benefit from a refactor. This patch is a stop-gap
to fix the CI with minimal changes.

Signed-off-by: Ihar Hrachyshka <[email protected]>
@booxter booxter changed the title fix: model.forward no longer accepts return_dict fix: model.forward now accepts return_dict via kwargs Apr 7, 2025
@booxter booxter marked this pull request as ready for review April 7, 2025 23:31
@mergify mergify bot added the ci-failure label Apr 7, 2025
@booxter
Copy link
Contributor Author

booxter commented Apr 8, 2025

e2e fails with Invalid chat template path: None. Is it a known issue?

@booxter
Copy link
Contributor Author

booxter commented Apr 8, 2025

Apparently e2e failure was supposed to be fixed by instructlab/instructlab#3254 but since this PR fixes functional tests for that other PR, we have chicken and egg. I guess instructlab will have to pin / cap transformers library first; then we can later land this change here; then finally bump in instructlab and uncap transformers.

Copy link
Contributor

@JamesKunstle JamesKunstle left a comment

Choose a reason for hiding this comment

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

lgtm!

@mergify mergify bot added the one-approval label Apr 8, 2025
@RobotSail RobotSail merged commit 694ca16 into instructlab:main Apr 8, 2025
15 of 16 checks passed
@mergify mergify bot removed the one-approval label Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants