Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding MSELoss for regression tasks in HubertForSequenceClassification model #33551

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Adding MSELoss for regression tasks in HubertForSequenceClassification model #33551

wants to merge 9 commits into from
+21 −3

Conversation

aroun-coumar
Copy link
Contributor

What does this PR do?

Fixes #33500

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 or the forum? 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, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@ylacombe
@eustlb

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.

@aroun-coumar aroun-coumar mentioned this pull request Sep 18, 2024
Closed
4 tasks
@brayanrodbajo
Copy link

Any news on this PR?

@aroun-coumar
Copy link
Contributor Author

Gentle ping @ylacombe !

ylacombe
ylacombe approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@ylacombe ylacombe left a comment

Choose a reason for hiding this comment

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

This LGTM!

Could you maybe add the use-case multi_label_classification while you're at it? It's done here as well

@ylacombe
Copy link
Contributor

I haven't realized, but as you can see with the failing check_repository_consistency, the model copies some code from Wav2Vec2ForSequenceClassification, which you also have to modify!

You then need to pass on the changes to all models copying Wav2Vec2ForSequenceClassification by doing make fix-copies!
We do that to ensure consistency of the repository. That way, your fix will be apply to a lot of audio models as well!

@aroun-coumar
Copy link
Contributor Author

Hey @ylacombe i am stuck with this error. Please help me.
Thanks!

(myvenv)
Arun kumar@LAPTOP-0U925LO8 MINGW64 ~/OneDrive/desktop/Transformers/transformers (fix-classification-loss)
$ make fix-copies
python utils/check_copies.py --fix_and_overwrite
Detected changes, rewriting src/transformers\models\hubert\modeling_hubert.py.
python utils/check_modular_conversion.py  --fix_and_overwrite
Traceback (most recent call last):
  File "C:\Users\Arun kumar\OneDrive\desktop\Transformers\transformers\utils\check_modular_conversion.py", line 73, in <module>
    non_matching_files += compare_files(modular_file_path, args.fix_and_overwrite)
  File "C:\Users\Arun kumar\OneDrive\desktop\Transformers\transformers\utils\check_modular_conversion.py", line 53, in compare_files
    generated_modeling_content = convert_modular_file(modular_file_path)
  File "C:\Users\Arun kumar\OneDrive\desktop\Transformers\transformers\utils\modular_model_converter.py", line 1136, in convert_modular_file
    code = file.read()
  File "C:\Users\Arun kumar\AppData\Local\Programs\Python\Python39\lib\encodings\cp1252.py", line 23, in decode
    return codecs.charmap_decode(input,self.errors,decoding_table)[0]
UnicodeDecodeError: 'charmap' codec can't decode byte 0x81 in position 1699: character maps to <undefined>
make: *** [Makefile:84: fix-copies] Error 1

@aroun-coumar
Copy link
Contributor Author

Is this my local setup problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brayanrodbajo brayanrodbajo brayanrodbajo approved these changes

@ylacombe ylacombe Awaiting requested review from ylacombe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HubertForSequenceClassification does not handle regression tasks correctly; always uses CrossEntropyLoss
3 participants
@aroun-coumar @brayanrodbajo @ylacombe