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.
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
add initial design for uniform processors + align model #31197
add initial design for uniform processors + align model #31197
Changes from 1 commit
b85036f
bb8ac70
cd8c601
f00c852
693036f
766da3a
844394d
c19bbc6
3c38119
81ae819
ce4abcd
3acdf28
404239f
603be40
71c9d6c
26383c5
4521f4f
b96eb64
9c5c01c
3ccb505
142acf3
60a5730
be6c141
84135d7
ce967ac
f78ec52
52fd5ad
8f21abe
d510030
fd43bcd
b2cd7c9
bcbd646
39c1587
1f73bdf
b3f98ba
e4d6d12
1e09e4a
d4232f0
162b1a7
0da1dc3
f955510
f6f1dac
c4b7e84
3ce3608
6b83e39
3818b86
31b7a60
4072336
bcce007
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
should work no? Or does it not update the default for type-hints?
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.
yes it works for sure, this was to have a structured dict for defaults. Can change :)
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.
ah, now I remember, it actually can't work like that since Typed Dicts don't support default values, they are made to hold the layout. They can have any attributes however, but it won't pass a value as default -like a
dataclass
would, but in this case we'd lose typing-, hence the manual operationThere 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.
ok got it thanks! Let's maybe comment about this!
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 we have a comment for future code inspectors? I'm assuming here isn't the best place (we don't want it for all models) but didn't find a corresponding one elsewhere on a quick skim
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.
On that: there's doc in
processing_utils.ProcessingKwargs
, I added a comment nudging users to check there for documentation!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.
Might there be a situation when we have the key in kwargs and modality kwargs?
I am getting a strange if key is provided the same key for both:
I guess the priority should working here or a proper error raised.
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.
ah yes, good catch! I think raising an error if both keys are defined is easier - responsibility of choosing should be on the user side imo
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.
Should be fixed :) example you included would raise a
ValueError