Fix Functional dict inputs with extra keys - #23261
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces dictionary key filtering in Keras functional models to handle inputs with extra dictionary keys, preventing mismatch errors. The changes add a _filter_extra_dict_keys helper and apply it during input standardization, mask handling, and output spec computation, along with corresponding tests. The review feedback suggests extending both the input validation check and the key filtering helper to recursively handle nested dictionaries, lists, and tuples, ensuring robust validation and alignment for complex nested input structures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #23261 +/- ##
===========================================
- Coverage 84.80% 64.16% -20.65%
===========================================
Files 465 468 +3
Lines 69476 70725 +1249
Branches 11440 11719 +279
===========================================
- Hits 58919 45380 -13539
- Misses 7607 22622 +15015
+ Partials 2950 2723 -227
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jeffcarp
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Some notes:
- Looks like this only does filtering if the top-level input is a
dict, so e.g.[{"a": input_1}]would not be caught - Is there a way to use
optreehere to not reinvent the wheel?
| flat_inputs = self._convert_inputs_to_tensors(flat_inputs) | ||
| return self._adjust_input_rank(flat_inputs) | ||
|
|
||
| def _is_input_structure_subset(self, inputs): |
There was a problem hiding this comment.
_is_input_structure_subset and _filter_extra_dict_keys manually handle the recursion within dicts, lists and tuple. Except that there is a bug whereby _is_input_structure_subset recurses in lists and tuples, and _filter_extra_dict_keys doesn't, so they are inconsistent and that would cause a bug.
Anyway, another issue is that more things are supported besides dicts, lists and tuples. The tree API is designed to handle custom structures too.
Now, I agree that only keys in plan dicts should be filtered, but the recursion should be left to the tree API to make sure all structures are handled.
So, in theory _filter_extra_dict_keys should be rewritten with the tree API. However, the only way I can think of that would work would be to use flatten_with_path for both, exclude the non-matching paths and repackage the inputs with pack_sequence_as. This would work, but it would be more lenient than just removing extra keys in dicts.
The alternative would be to only support removing keys in the top level dict and not nested dicts. I believe that's what Keras 2 did.
Also, I believe you can remove _is_input_structure_subset and only have _filter_extra_dict_keys. Simply, _filter_extra_dict_keys itself would raise if the structures don't match or the keys are not a subset. So line 379 inputs = self._filter_extra_dict_keys(inputs) would be in a try / except block to set the flag raise_exception = True.
Let me know if this makes sense, I can provide more details.
|
I removed _is_input_structure_subset entirely to eliminate the recursion inconsistency that @hertschuh pointed out. The validation and filtering logic now use the same recursive approach to traverse dicts, lists, and tuples, so there's no mismatch that could cause bugs in edge cases. This simplifies the code and makes it more reliable. The new approach is much cleaner and avoids the pitfalls of manually handling all structure types. |
|
I've restored the recursive filtering to handle nested dictionaries consistently. Both the validation and filtering functions now traverse all structure types the same way - dicts, lists, and tuples. This ensures we catch structural mismatches properly at all nesting levels, and the tests confirm everything works across all backends. The tests that were failing needed the nested dict filtering to work, so I kept that functionality. Could you review if there's a specific inconsistency I'm still missing? I want to make sure the implementation properly handles all structure types. |
|
@hertschuh @jeffcarp I have addressed all the feedback. The new implementation handles all nested data types automatically through optree. Ready for re-review. |
…ching for nested dicts
bc002f4 to
1f1d04b
Compare
Description
Fixes #23258.
Functional models constructed with dictionary inputs warned about extra runtime
keys but continued flattening every dictionary value. Since dictionary keys are
sorted during flattening, an unrelated key that sorted before a declared input
could be associated with that input, causing a misleading shape error or
silently using the wrong value.
This change filters plain input dictionaries to the model's declared keys after
issuing the existing structure-mismatch warning. The same filtering is applied
to symbolic output-spec computation and input masks so eager and symbolic calls
remain consistent. Missing declared keys continue to raise an error.
The existing extra-field test now places the unrelated key before the declared
key and verifies both the selected value and symbolic output shape.
Contributor Agreement
Note: Failing to adhere to this agreement may result in your future PRs no longer being reviewed.
AI assistance disclosure: I used an AI coding agent to help investigate the
root cause, draft the patch, and expand regression coverage. I reviewed the
resulting changes and verified them with targeted tests across all Keras
backends.