[DNMY] Per-type service models and merged sparse/dense reserve containers - #141
[DNMY] Per-type service models and merged sparse/dense reserve containers#141rodrigomha wants to merge 10 commits into
Conversation
|
|
||
| # Service model | ||
| # | ||
| # Services of the same type now share merged constraint containers keyed by |
There was a problem hiding this comment.
I'm thinking of cleaning up these comments, but I will leave them for now for the review, but everything referring to how it was before I plan to eventually remove it
|
Performance Results This branch |
There was a problem hiding this comment.
Pull request overview
Refactors the service-model layer to be per service type (one ServiceModel per service type) and updates reserve-style variables/constraints to use merged sparse/dense containers, enabling shared sparse (service, device, time) storage. This is the upstream half of a coordinated breaking change intended to be consumed by a follow-up migration in downstream packages.
Changes:
- Replace tuple-keyed
ServicesModelContainerwithDict{Symbol, ServiceModel}keyed bySymbol(service_type), and removeservice_name/get_service_namefromServiceModel. - Introduce sparse 3D container support for
EmulationModelStore.write_output!(with a new round-trip unit test). - Update service variable/dual container construction to reflect merged container strategy (including dual assignment mirroring existing constraint containers).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/test_emulation_model_store.jl | Adds a sparse 3D write/read round-trip test to validate emulation store behavior for reserve-style containers. |
| src/operation/problem_template.jl | Updates ServicesModelContainer type alias to the new per-type Dict{Symbol, ServiceModel} shape. |
| src/operation/emulation_model_store.jl | Adds write_output! support for SparseAxisArray by flattening leading tuple axes into encoded columns. |
| src/InfrastructureOptimizationModels.jl | Removes get_service_name from exports to match API removal. |
| src/core/service_model.jl | Removes per-service-name ServiceModel identity; introduces per-service nested contributing-device mapping and new accessors. |
| src/core/definitions.jl | Removes the vestigial NO_SERVICE_NAME_PROVIDED constant. |
| src/common_models/add_variable.jl | Updates service variable creation to use a merged sparse (service, device, time) container. |
| src/common_models/add_constraint_dual.jl | Updates service dual assignment to mirror merged constraint containers rather than per-service meta axes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
luke-kiernan
left a comment
luke-kiernan
left a comment
There was a problem hiding this comment.
This area of the code is a can of worms when it comes to type stability. Not really a new issue, but still worth at least adding a function barrier and some type labels.
| time_steps = get_time_steps(container) | ||
| for constraint_type in get_duals(model) | ||
| assign_dual_variable!(container, constraint_type, service, D) | ||
| for key in _existing_constraint_keys(container, constraint_type, T) |
There was a problem hiding this comment.
Do a type stability audit on these nested loops. Thoughts:
duals::Vector{DataType}, so all the compiler knows isconstraint_type::DataType. Can we give it something more specific?- similar considerations for
key - introduce a function barrier?
get_entry_type(key): I suspect that's justconstraint_type
Other ways to make this more type stable and compiler-friendly....maintain a list of all meta's for each (constraint_type, component_type) combination? Then we could loop over those (a compile time value) and skip those that aren't present, instead of accumulating the ones that are present via _existing_constraint_keys (a runtime value).
There was a problem hiding this comment.
You are right about the get_entry_type. I will fix that.
I ask Claude about the instability and the warntype and:
@code_warntype does show constraint_type::DataType, key::ConstraintKey, existing::Any, dual_key::Any — but I tried a barrier parameterized on the concrete (constraint_type, component_type) and key/existing/dual_key stay Any. The roots are structural: constraints is an OrderedDict{ConstraintKey, JuMPArray} with abstract key/value types, and the ConstraintKey constructor doesn't infer concretely — so a barrier localizes the constraint_type dispatch but doesn't reach stability, and the Dense-vs-Sparse dispatch that actually matters is already a multiple-dispatch barrier. Since this runs once per build over a handful of dual types/keys, I'd propose taking the constraint_type cleanup now and skipping the meta-registry.
I agree with Claude here, the effort is larger than the sparse refactoring here. Are you ok with this @jd-lara?
There was a problem hiding this comment.
This is not a performance critical operation, it is more of a convenience and for now works correctly.
| Dict{DataType, Vector{<:IS.InfrastructureSystemsComponent}}(), | ||
| ) | ||
| # All contributing devices across ALL services (flatten the nested map). | ||
| get_contributing_devices(m::ServiceModel) = |
There was a problem hiding this comment.
Flattening makes this heterogeneous--a mix of different subtypes of IS.InfrastructureSystemsComponent--hence type unstable to iterate over. There are ways around this...but it'd add complexity. You'd be limited to map, foreach, etc. (for loops would remain unstable) and we might need to add one or two @generated functions
There was a problem hiding this comment.
This is true if a service have different types for contributing devices (e.g. Hydro + Thermal). However, if all the contributing devices are the same type, then it narrows to a concrete Vector{CommonType} (e.g. Vector{ThermalStandard}) and iterates stably.
I asked Claude about this and basically:
only mixed-type reserves give an abstract join, and even then it's build-time and JuMP-@variable-dominated (dispatch ≈ 1.6% of per-device cost, ~60× under the allocation).
After some discussion I think this is a temporary solution so we don't implement ton of complexity here
Two spots flatten the device map for no good reason, and fixing them is pure allocation removal with no behavior or stability question:
services_constructor.jl:37,77— these guard construction with isempty(get_contributing_devices(service_model)), which builds a full flattened vector of every contributing device across all services just to ask "is it empty?". Replace with an emptiness check on the nested map itself (it's empty iff the map has no entries) — no allocation.problem_template.jl:303 (_modify_device_model!)— this only needs the set of device types (Set{DataType}(typeof.(devices))) to wire the service into the right DeviceModels. It can read the map's keys directly instead of flattening and re-deriving typeof on each element.These touch neither the hot loops nor the container shapes, so there's no test risk. They're worth doing regardless of what we decide about the stability fix.
What you want is to modify our flatten map and use the map per type as follows:
The deferred form iterates the per-type groups the map already holds, so each group is a runtime-concrete vector and the barrier (add_service_variables!) specializes per group:
for (device_type, devices) in get_contributing_devices_map(model, service_name) # devices::Vector{ThermalStandard}, etc.
add_service_variables!(container, ActivePowerReserveVariable, service, devices, F) # D binds concretely
endI also asked Claude about why this is a major change and basically:
- It changes the container merge cadence. add_service_variables! is currently called once per service and appends one slice to the shared sparse (service, device, time) container via lazy_container_addition!. Per-group, it's called once per device type per service, appending finer slices. That's semantically fine (each call fills its own (service, device, t) keys), but it changes the create/append sequence and has to be re-verified against the existing tests — it's a behavior-adjacent change, not a comment tweak.
- It's not one call site. The same flatten-then-loop shape feeds _sum_service_reserves (inside RequirementConstraint) and the ParticipationFraction / ramp / reserve-power builders. To actually recover stability you'd convert all of them to accumulate per group into the shared expression/constraint. That's a handful of reserve builders touched, so more surface and more test risk.
- The payoff is small and conditional. The common case (single contributing-device type per reserve) is already stable — the flatten narrows to a concrete Vector{ThermalStandard} and add_service_variables! is already a barrier. The per-group change only helps mixed-type reserves, and even there it's build-time-only, with dynamic dispatch measured at ~1.6% of per-device cost (JuMP
@variableallocation dominates ~60×).So: real stability improvement, but touching several builders + the container merge cadence for a marginal, build-time-only, mixed-reserve-only gain. That ratio says "tracked follow-up," not "block this PR." I'd file it as an issue (and it slots naturally next to the interface-migration follow-ups, since it's the same "iterate the map by type" shape).
I agree that this is better it could be changing too much the structure that we have for slicing sparse containers. What do you think @jd-lara
There was a problem hiding this comment.
if we want to fix this to be properly type stable we need to change PSY to have a mapping by type correctly. I think we need to get this logic correct and then make the performance improvement work. I prefer we make it work and open an issue
|
@luke-kiernan I think you have valid points, but we need to make the container refactoring work correctly which a massive change and then we can address the heterogeneous types in the services problem |
Sounds good. Yeah it's not directly in a hot path. |
rodrigomha
changed the title
| Dict{DataType, Vector{<:IS.InfrastructureSystemsComponent}}() | ||
| # One service's inner `Dict{DataType, Vector}` (shared empty Dict if the service is absent). | ||
| get_contributing_devices_map(m::ServiceModel, service_name::AbstractString) = | ||
| get(() -> _EMPTY_CONTRIBUTING_DEVICES_MAP, m.contributing_devices_map, service_name) |
There was a problem hiding this comment.
Do we need the anonymous function here? This seems an odd way to create an empty dictionary
There was a problem hiding this comment.
I asked Claude the same, and basically told me that is like a micro optimization to avoid allocating. I asked specifically and showed me that you have less allocations. This was done when the const empty dict was used to avoid allocating an empty dict.
There was a problem hiding this comment.
Here is the answer, so it seems that for this case we don't need to use the anonymous function. I will revert for having more simplicity in the code.
get with eager vs. lazy default in Julia
The core reason: get(collection, key, default) evaluates default eagerly, before the lookup happens — every single call, whether the key is found or not. get(f, collection, key) (the anonymous-function form) only calls f when the key is missing — it's lazy.
# Eager: Dict() gets allocated on EVERY call, even on a hit
get(m.contributing_devices_map, service_name, Dict())
# Lazy: closure only runs on a miss
get(() -> Dict(), m.contributing_devices_map, service_name)This matters a lot when the default is expensive to construct — a new Dict(), a computed value, a function call, string interpolation, etc. In a hot loop, the eager version allocates garbage on every hit even though you throw it away immediately.
In this specific case
_EMPTY_CONTRIBUTING_DEVICES_MAP looks like a preallocated global constant, not something built fresh each call. So:
get(m.contributing_devices_map, service_name, _EMPTY_CONTRIBUTING_DEVICES_MAP)just evaluates a variable reference — no allocation either way. And () -> _EMPTY_CONTRIBUTING_DEVICES_MAP captures nothing, so Julia compiles it to a stateless singleton closure with no extra allocation either.
Takeaway
For this exact line, the anonymous-function version isn't meaningfully faster — the "optimization" advice is more relevant when the default is constructed on the spot (e.g., Dict(), a comprehension, string building). That said, it's a reasonable defensive habit: if someone later changes the default to something more expensive, the closure form protects you automatically without needing to remember to change call sites.
There was a problem hiding this comment.
it seems WAY easier to use a haskey than defining an anonymous function like that. It feels very pythonic as an approach. Use a closure like that
…VICE_NAME_PROVIDED const
The 3-arg get(dict, key, default) evaluates its default eagerly, so the per-service map accessor allocated a throwaway empty Dict on every call, including the hit path. Return a shared const empty sentinel via the lazy get(f, dict, key) form instead; the accessor is read-only for all callers (only the no-arg whole-map form is mutated via get!), so sharing is safe. Also flag the two flattening get_contributing_devices methods with a TODO: flattening across multiple contributing device types widens the element type and costs downstream type stability; revisit by iterating the concretely-typed per-device-type map groups.
…dundant closure) The lazy-default closure guarded against an allocation that cannot happen: the empty default is already a const, so the 3-arg get references it rather than rebuilding it. Confirmed 0 alloc on hit and miss paths.
…containers
The SparseVariableType auto-created container was hardcoded to the 3D device-offer PWL key
`(device_name, segment, time)`. Introduce `sparse_variable_key_type(::Type{<:SparseVariableType})`
returning that tuple by default, and have `_get_pwl_variables_container` build the empty
SparseAxisArray from it. Downstream packages override the trait for variable types that need a
different key shape - e.g. a per-service reserve offer keyed
`(service_name, device_name, segment, time)`.
No behavior change for existing sparse variable types (default returns the prior 3-tuple).
e82ddf1 to
12b3835
Compare
The Documentation CI failed (red on main too) for two reasons: - docs/Project.toml had no [sources], so the docs env resolved InfrastructureSystems from the registry, which lacks InfrastructureMatrices (IOM imports it) -> IOM failed to precompile. Pin InfrastructureSystems to IS4, matching the root and test envs. - After that, makedocs terminated on :cross_references: explanation/reference pages @ref symbols moved to PowerOperationsModels in the IOM/POM split (#104). warnonly on :cross_references only, so the site builds while missing-docstring and doctest checks stay strict.
Domain-neutral IOM changes backing POM's per-type service-model refactor: one
ServiceModelper service type (not per service name), with merged sparse/dense reserve containers keyed by an added service axis.Rebased on latest
main(includes #142); tests pass against the latest IS4 (v3.6.0).What changed
Per-type ServiceModel API + per-service sparse variables
ServiceModelnow covers every service of its type; service variables are sparse containers with the service name as an added axis: reserve award is 3D(service, device, time), requirement-side is 2D(service, time).aggregated_service_modelattribute and the unusedNO_SERVICE_NAME_PROVIDEDconst.sparse_variable_key_typetrait (extension seam)sparse_variable_key_type(::Type{<:SparseVariableType})returns the auto-created container's key tuple, defaulting to the 3D device-offer shape(device, segment, time)._get_pwl_variables_containerbuilds the emptySparseAxisArrayfrom it.(service, device, segment, time)per-device reserve-offer block variable. No behavior change for existing sparse variable types (default returns the prior 3-tuple).Allocation / clarity cleanups
get_contributing_devices_map(m, name)uses a plain 3-arggetwith a shared empty-map const (no anonymous closure, no per-call empty-Dictallocation on the hit path).constraint_typedirectly (get_entry_type(key)was redundant); reworded the stale "grouped construction" dual comment (no grouping remains under per-type).Deferred follow-up
ServiceModel.contributing_devices_mapandDeviceModel.serviceshave abstract element types, so per-type service construction dynamic-dispatches at build time. Tracked in POM issue #216 (the fix needs an IOM struct-typing pass here, coordinated with the POM adoption).Testing
Full IOM suite green under
julia --project=test test/runtests.jlwith IS4 v3.6.0.Consumer
Backs POM #206, which now consolidates the full service refactor (Phases A+B+C — the previously stacked POM #207 and #210 have been folded into #206). The
sparse_variable_key_typeoverride is used there by the per-device reserve-offer (service bids) work. Merge order: this PR (or its content) settles first, then POM #206 repoints its[sources]off therh/dev_service_refactorbranch.