-
Notifications
You must be signed in to change notification settings - Fork 33.1k
Fix(43240): pass kwargs to nn.functional.cross_entropy #43251
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.
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
Open
jasiecky
wants to merge
22
commits into
huggingface:main
Choose a base branch
from
jasiecky:fix/43242
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
084eb5c
added validating kwargs passed to nn.functional.cross_entropy
jasiecky 51ad984
rollback
jasiecky 763fabd
removed not allowed kwargs
jasiecky 5225729
moved to inspect
jasiecky ee43e8f
added allowed_kwargs variable
jasiecky aa8d0ac
added tests
jasiecky 3f7f007
reduplicated code
jasiecky 29eb45b
Merge branch 'main' into fix/43242
jasiecky 660f71e
Merge branch 'main' into fix/43242
jasiecky f6c526f
Merge branch 'main' into fix/43242
jasiecky 57f3778
Merge branch 'main' into fix/43242
jasiecky 0203462
added only supported parameters
jasiecky b022749
Merge branch 'fix/43242' of https://github.com/jasiecky/transformers …
jasiecky 7b1e6af
removed unused imports
jasiecky fceddcf
changed label_smoothing to float
jasiecky 699ff0d
added kwargs
jasiecky 05fb96e
Merge branch 'main' into fix/43242
jasiecky d3838c9
Merge branch 'main' into fix/43242
jasiecky 294d851
changed to _kwargs
jasiecky f651994
Merge branch 'fix/43242' of https://github.com/jasiecky/transformers …
jasiecky 6e287b1
Merge branch 'main' into fix/43242
jasiecky 2395023
Merge branch 'main' into fix/43242
jasiecky File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh?
_?I'd say remove it altogether, since it's being silently ignored and that's bad for the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to remove kwargs? You accepted the code containing them;) If we don't use them the function isn't compatible with some parts of the repo so I changed it to _kwargs in order to explicitly show that kwargs're ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea how renaming to
_kwargsimplies that it is ignored. When something is ignored it shouldn't be there.As I shared earlier my opinion is that if
**kwargsis in the API, they should be introspected and any unexpected keys should be asserted on.**kwargsare useful when a function is an intermediary and passes it on. In this casekwargsaren't passed on and thus shouldn't be there.I'm not a current maintainer so my vote isn't binding. You want to engage current maintainers instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a naming convention, it doesn't imply anything indeed. Let's wait for the mainteners;)
@iamsernine @ArthurZucker @cyyever