fix: normalize None timesteps and use .get() for safe optim_conf reads#842
Open
hossamnagy wants to merge 1 commit into
Open
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideNormalizes deferrable timestep parameter lists by replacing None values with safe defaults both at validation time and during optimization, preventing TypeError crashes when corrupted or partially specified configurations are processed. Sequence diagram for deferrable timestep normalization and validationsequenceDiagram
actor ExternalCaller
participant set_config_endpoint
participant check_def_loads
participant params_store
participant perform_optimization
participant validate_def_timewindow
ExternalCaller->>set_config_endpoint: send partial_config ([null, 0])
set_config_endpoint->>check_def_loads: check_def_loads(parameter, parameter_name, num_def_loads, default)
check_def_loads-->>set_config_endpoint: normalized_list ([0, 0])
set_config_endpoint->>params_store: write params.pkl (normalized_list)
ExternalCaller->>perform_optimization: trigger optimization
perform_optimization->>params_store: read params.pkl
perform_optimization->>perform_optimization: pad_list(def_start_timestep, num_def_loads)
perform_optimization->>perform_optimization: pad_list(def_end_timestep, num_def_loads)
perform_optimization->>perform_optimization: normalize None to 0 in def_start_timestep
perform_optimization->>perform_optimization: normalize None to 0 in def_end_timestep
perform_optimization->>validate_def_timewindow: validate_def_timewindow(start, end, operating_hours, idx)
validate_def_timewindow-->>perform_optimization: validation_result (no TypeError)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
perform_optimization, consider moving theNone→0 normalization intopad_list(or a small shared helper) so that all padded configuration lists benefit from the same defensive handling rather than onlydef_start_timestepanddef_end_timestep. - In
check_def_loads, theisinstance(result, list)guard suggestsparameter[parameter_name]might not always be a list; if that's truly unexpected for this helper, you may want to assert or raise early to catch misuses instead of silently returning a non-normalized value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `perform_optimization`, consider moving the `None`→0 normalization into `pad_list` (or a small shared helper) so that all padded configuration lists benefit from the same defensive handling rather than only `def_start_timestep` and `def_end_timestep`.
- In `check_def_loads`, the `isinstance(result, list)` guard suggests `parameter[parameter_name]` might not always be a list; if that's truly unexpected for this helper, you may want to assert or raise early to catch misuses instead of silently returning a non-normalized value.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
7610100 to
fe90fd9
Compare
fe90fd9 to
16cf11f
Compare
Three targeted fixes for a TypeError crash in naive_mpc_optim, verified
live on EMHASS 0.17.2.
**utils.py — normalize None in check_def_loads (root cause)**
When /set-config receives a partial payload it can persist [null, 0] for
start_timesteps_of_each_deferrable_load. check_def_loads only pads a
short list — it never replaces None values already inside the list. After
this fix, any None element is replaced with the supplied default before
the value is returned and re-saved to params.pkl.
**optimization.py — defensive None→0 after pad_list (second layer)**
Adds a guard after pad_list() in perform_optimization() that replaces any
residual None with 0 (= "no time restriction"). Protects installations
whose params.pkl was already corrupted before the utils fix.
Without these two fixes the crash is:
TypeError: '<=' not supported between instances of 'NoneType' and 'int'
File "emhass/optimization.py", in validate_def_timewindow
if start <= end or start <= 0 or end <= 0:
Reproduces when operating_hours_of_each_deferrable_load = [0, 0] (both
loads disabled) and start_timesteps contains a None element.
**command_line.py — use .get() for timestep reads (KeyError prevention)**
def_start_timestep and def_end_timestep were read with bare [] which
raises KeyError if the key is absent from optim_conf. Changed to .get(),
consistent with how operating_hours and operating_timesteps are already
read in the same block.
Tested on a live HA installation: reproduced the crash, confirmed
params.pkl contained [None, 0], applied patch, subsequent cycles with
operating_hours=[0,0] complete with Status: Optimal.
16cf11f to
3398bd2
Compare
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.
Summary
Three targeted fixes for a
TypeErrorcrash innaive_mpc_optim, verified live on EMHASS 0.17.2 running as a standalone Docker container against Home Assistant.1.
utils.py— normalizeNoneelements incheck_def_loads(root cause fix)When
/set-configreceives a partial config payload it can persist a list like[null, 0]forstart_timesteps_of_each_deferrable_load.check_def_loadsonly pads a short list — it never replacesNonevalues that are already inside the list. After this fix, anyNoneelement is replaced with the supplieddefaultbefore the value is returned and re-saved.2.
optimization.py— defensiveNone→0afterpad_list(second layer)Adds a guard immediately after the
pad_list()calls inperform_optimization()that replaces any residualNonewith0(= "no time restriction"). Protects installations whoseparams.pklwas already corrupted before fix 1 was applied.Without fixes 1 & 2 the crash is:
Reproduces when
operating_hours_of_each_deferrable_load = [0, 0](both loads disabled) andstart_timestepscontains aNoneelement.3.
command_line.py— use.get()for timestep reads (KeyError prevention)def_start_timestepanddef_end_timestepwere read fromoptim_confusing bare[]which raisesKeyErrorif the key is absent. Changed to.get()— consistent with howoperating_hours_of_each_deferrable_loadandoperating_timesteps_of_each_deferrable_loadare already read in the same block.Reproduction (fixes 1–3)
num_def_loads = 2)./set-configwithstart_timesteps_of_each_deferrable_load: [null, 0].check_def_loadsseeslen == num_def_loads, skips padding, returns[None, 0]unchanged.pad_listinperform_optimizationalso skips (length already correct)./action/naive-mpc-optimwithoperating_hours_of_each_deferrable_load: [0, 0]→validate_def_timewindow(None, 0, 0, n)→ crash.Testing
Verified on a live Home Assistant installation (EMHASS 0.17.2, Docker, InfluxDB retrieval):
params.pklcontained[None, 0]forstart_timesteps_of_each_deferrable_load.operating_hours = [0, 0]complete withStatus: Optimal.operating_hours > 0.