fix KeyError in VAE cache disabling - #2997
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a KeyError risk around VAE cache configuration by centralizing normalization of vae_cache_disable / vae_cache_ondemand flags in the data backend factory, and extends edge-case tests to cover scenarios where config keys are absent.
Changes:
- Add
_normalise_vae_cache_configto coerce optional VAE cache flags and enforce “disable implies on-demand”. - Update factory logic to use the normalization helper instead of direct config indexing when configuring/counting cache behavior.
- Expand the edge-case test matrix to include scenarios with missing VAE cache config keys.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
simpletuner/helpers/data_backend/factory.py |
Adds and applies a VAE cache config normalization helper to avoid missing-key access and keep flag semantics consistent. |
tests/test_factory_edge_cases.py |
Extends the VAE cache mode tests to exercise behavior when specific config keys are removed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+268
to
+269
| for key in missing_config_keys: | ||
| init_backend["config"].pop(key) |
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.
This pull request refactors how VAE cache configuration is normalized and accessed throughout the codebase, ensuring consistent handling of related config flags and improving test coverage for edge cases. The main changes introduce a helper function to centralize normalization logic, update all relevant usages, and enhance test cases to cover missing config keys.
VAE Cache Configuration Normalization:
_normalise_vae_cache_configinfactory.pyto consistently interpret and update thevae_cache_disableandvae_cache_ondemandflags, ensuring that disabling the cache implies on-demand mode._configure_vae_cacheand_count_entriesto use the new normalization function instead of directly accessing the config dictionary. [1] [2]Test Improvements:
test_dataset_vae_cache_modes_use_dataset_ondemand_modeto include cases with missing config keys, ensuring robust behavior when certain flags are absent. [1] [2]