Repository navigation
fix(group): apply config when require_array returns an existing array - #4413
Merged
Merged
Conversation
require_array forwarded **kwargs, including config, only to create_array in the branch that creates a missing array. Requiring an existing array dropped config and returned it with the global defaults. Apply it with with_config(), as open_array does since zarr-developers#4408. Assisted-by: ClaudeCode:claude-opus-5-5
Assisted-by: ClaudeCode:claude-opus-5-5
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4413 +/- ##
=======================================
Coverage 94.44% 94.45%
=======================================
Files 93 93
Lines 13221 13224 +3
=======================================
+ Hits 12487 12491 +4
+ Misses 734 733 -1
🚀 New features to boost your workflow:
|
config was read from **kwargs, which is how the existing-array branch came to drop it. It is now a named parameter of Group.require_array and AsyncGroup.require_array, passed explicitly to create_array for a new array and applied with with_config() to an existing one. The existing array comes from getitem, which serves member lookup in general (including consolidated metadata) and takes no runtime config. Assisted-by: ClaudeCode:claude-opus-5-5
d-v-b
marked this pull request as ready for review
September 30, 2026 08:02
The existing-array branch handed config to with_config, which merged it over the array's current config and rejected an unknown key with the dataclass's "unexpected keyword argument" message. The create branch went through parse_array_config, which fills missing keys from the global configuration and says "Unknown array config keys". The same bad input now raises the same error whichever branch runs, and the docstring's "taken from the global configuration" holds by construction. The error tests run on both branches, and the combinations test passes config=None explicitly instead of omitting the keyword. Assisted-by: ClaudeCode:claude-fable-5-1
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Part of the kwargs audit, here's a parameter (config) not being propagated correctly through the chain of function calls. this PR fixes that.
🤖 AI text below 🤖
Group.require_array(sync and async) forwarded**kwargs, includingconfig, only tocreate_arrayin the branch that creates a missing array. When the array already existed,configwas silently dropped:This is the same bug as #4408 (
open_array), found in the same**kwargsaudit.Changes
configis a named parameter ofGroup.require_arrayandAsyncGroup.require_array, passed explicitly to both branches. Reading it from**kwargsis what let the existing-array branch drop it.configgoes tocreate_array..with_config(config). The existing array comes fromgetitem, which serves member lookup in general (including consolidated metadata) and takes no runtime config, so adding one there would be out of place.with_configreuses the metadata and only rebuilds the codec pipeline.configthroughparse_array_configbeforewith_config, so an unknown key raises the sameTypeError("Unknown array config keys: [...]")whether the array exists or is created, and missing keys come from the global configuration in both cases.Behaviour change: an invalid
configgiven for an existing array used to be ignored, and now raises (TypeErrorfor an unknown key,ValueErrorfor an invalid value).Tests
test_require_array_config: zarr format 2/3 × sync/async × existing/new array ×configasNone, a partial dict, a multi-key dict, or anArrayConfig.test_require_array_unknown_config_keyandtest_require_array_invalid_config_value: one test per error case, each run against an existing and a newly created array.14 of the new cases fail on
main. The full test suite passes locally, and so does mypy.Not covered
The other kwargs are still ignored for an existing array without a warning: stored-metadata arguments such as
fill_value,chunks,attributesandoverwrite, and unknown keys (which raise only on the create branch). How those should behave is an open question, and this PR doesn't answer it.🤖 Generated with Claude Code