fix: stop using deprecated tsam ClusterConfig(weights=) API#689
Merged
Conversation
Clustering passed weights via `ClusterConfig(weights=...)`, which tsam
>= 3.4.0 deprecates with a DeprecationWarning. Weights are now resolved
separately and passed as a top-level argument to `tsam.aggregate()`.
This broke docs publishing: the clustering notebooks execute under a
warnings-as-error filter that `demandlib` leaks globally (it calls
`warnings.simplefilter("error")` and never restores it), so the
DeprecationWarning became a fatal CellExecutionError. The docs `build`
job failed, and since `deploy` needs `build`, the site stayed pinned at
6.1.0 (6.1.1-6.1.3 never published).
Also:
- Contain demandlib's global warnings poisoning at its source by
wrapping the call in `warnings.catch_warnings()`, so third-party
warnings can no longer turn notebook cells into hard errors.
- Run the docs build on PRs that change `flixopt/**` (notebooks import
flixopt), so a library change that breaks the docs is caught at PR
time instead of only after merge.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThree independent changes: GitHub Actions workflow expanded to trigger documentation builds on library changes; warning handling isolated in a notebook to contain demandlib side effects; and clustering logic refactored to extract weights from ClusterConfig for direct tsam.aggregate integration. ChangesDocumentation Workflow Trigger
Warning State Management
Clustering Weight Separation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/transform_accessor.py (1)
1800-1814:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
'weights'toreserved_tsam_keysto avoid a duplicate-keyword crash.Now that
weights=resolved_weightsis passed explicitly, any user who routesweightsthrough**tsam_kwargs(Line 1813) — plausible sinceClusterConfig(weights=...)is deprecated — will hitTypeError: aggregate() got multiple values for keyword argument 'weights'. The pre-flight guard at Lines 1739-1744 won't catch it becauseweightsis missing fromreserved_tsam_keys.🛡️ Reserve the key so the guard produces a clear error
reserved_tsam_keys = { 'n_clusters', 'period_duration', # exposed as cluster_duration 'timestep_duration', # computed automatically 'cluster', + 'weights', # resolved from ClusterConfig and passed explicitly 'segments', 'extremes', 'preserve_column_means', 'rescale_exclude_columns', 'round_decimals', 'numerical_tolerance', }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@flixopt/transform_accessor.py` around lines 1800 - 1814, The call to tsam.aggregate passes weights=resolved_weights explicitly while also forwarding **tsam_kwargs, which permits a duplicate 'weights' key from callers and causes TypeError; update the pre-flight guard by adding 'weights' to the reserved_tsam_keys collection referenced in this module so the existing validation (which checks keys in tsam_kwargs) will raise a clear error instead of letting aggregate receive duplicate keyword arguments (look for reserved_tsam_keys, tsam_kwargs, resolved_weights, and the aggregation_results = tsam.aggregate(...) call to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@flixopt/transform_accessor.py`:
- Around line 933-948: Replace the manual re-construction of ClusterConfig in
_build_cluster_config_with_weights with dataclasses.replace(cluster,
weights=None) to avoid re-listing fields (use ClusterConfig via
dataclasses.replace). Also ensure the code that builds
tsam_kwargs/reserved_tsam_keys excludes top-level 'weights' by adding 'weights'
to reserved_tsam_keys (so weights is passed only via the explicit weights=...
arg to tsam.aggregate and not via **tsam_kwargs), preventing "multiple values
for keyword argument 'weights'".
---
Outside diff comments:
In `@flixopt/transform_accessor.py`:
- Around line 1800-1814: The call to tsam.aggregate passes
weights=resolved_weights explicitly while also forwarding **tsam_kwargs, which
permits a duplicate 'weights' key from callers and causes TypeError; update the
pre-flight guard by adding 'weights' to the reserved_tsam_keys collection
referenced in this module so the existing validation (which checks keys in
tsam_kwargs) will raise a clear error instead of letting aggregate receive
duplicate keyword arguments (look for reserved_tsam_keys, tsam_kwargs,
resolved_weights, and the aggregation_results = tsam.aggregate(...) call to
locate the change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd657979-0bcd-4900-9bb4-1799dd3e4a0f
📒 Files selected for processing (3)
.github/workflows/docs.yamldocs/notebooks/data/generate_realistic_profiles.pyflixopt/transform_accessor.py
Comment on lines
+933
to
948
| # No ClusterConfig provided - use defaults (weights passed to aggregate()) | ||
| if cluster is None: | ||
| return ClusterConfig(weights=weights) | ||
|
|
||
| # ClusterConfig provided - use its settings with (possibly filtered) weights | ||
| return ClusterConfig( | ||
| method=cluster.method, | ||
| representation=cluster.representation, | ||
| weights=weights, | ||
| normalize_column_means=cluster.normalize_column_means, | ||
| use_duration_curves=cluster.use_duration_curves, | ||
| include_period_sums=cluster.include_period_sums, | ||
| solver=cluster.solver, | ||
| return ClusterConfig(), weights | ||
|
|
||
| # ClusterConfig provided - preserve its settings; weights passed to aggregate() | ||
| return ( | ||
| ClusterConfig( | ||
| method=cluster.method, | ||
| representation=cluster.representation, | ||
| normalize_column_means=cluster.normalize_column_means, | ||
| use_duration_curves=cluster.use_duration_curves, | ||
| include_period_sums=cluster.include_period_sums, | ||
| solver=cluster.solver, | ||
| ), | ||
| weights, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm ClusterConfig is a dataclass and weights=None does not warn
rg -nP --no-heading 'tsam' pyproject.toml setup.cfg setup.py requirements*.txt 2>/dev/null
pip install 'tsam==3.4.0' >/dev/null 2>&1 && python - <<'PY'
import dataclasses, warnings
from tsam import ClusterConfig
print("is_dataclass:", dataclasses.is_dataclass(ClusterConfig))
print("fields:", [f.name for f in dataclasses.fields(ClusterConfig)])
base = ClusterConfig(weights={'a': 1.0})
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter('always')
dataclasses.replace(base, weights=None)
print("warnings on replace(weights=None):", [str(x.category.__name__) for x in w])
PYRepository: flixOpt/flixopt
Length of output: 849
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect tsam==3.4.0 aggregate signature for weights=
python - <<'PY'
import inspect
import tsam
from tsam import aggregate
print("tsam version:", getattr(tsam, "__version__", "unknown"))
print("aggregate:", aggregate)
print("aggregate signature:", inspect.signature(aggregate))
PY
# 2) Try calling aggregate with weights= kwarg to see if it raises TypeError
python - <<'PY'
import numpy as np
import traceback
from tsam import aggregate
data = np.arange(12, dtype=float).reshape(6,2) # minimal 2-column time series-like data
kwargs = dict(
data=data,
n_clusters=2,
weights={"x": 1.0}, # if accepted, should not TypeError; if not, should complain
# keep other params default
)
try:
aggregate(**kwargs)
print("aggregate accepted weights= (call did not TypeError)")
except TypeError as e:
print("TypeError for weights=:", e)
except Exception as e:
# if it validates weights and fails later, still means weights kwarg is accepted
print("Non-TypeError raised (weights kwarg likely accepted):", type(e).__name__, e)
# print a short traceback line
tb = traceback.format_exc().splitlines()
print("traceback tail:", "\n".join(tb[-3:]))
PY
# 3) Repo checks around reserved_tsam_keys and cluster()/aggregate() call paths
rg -n "reserved_tsam_keys" -S flixopt/transform_accessor.py
rg -n "aggregate\\(" -S flixopt/transform_accessor.py
rg -n "weights" -S flixopt/transform_accessor.py | head -n 80
# Show the relevant sections (bounded)
python - <<'PY'
from pathlib import Path
p = Path("flixopt/transform_accessor.py")
lines = p.read_text().splitlines()
def show(start, end):
for i in range(start, end+1):
print(f"{i}:{lines[i-1]}")
print("\n--- reserved_tsam_keys region ---")
show(1700, 1760)
print("\n--- _build_cluster_config_with_weights region ---")
show(880, 980)
print("\n--- cluster() / aggregate() call region ---")
show(1760, 1860)
PYRepository: flixOpt/flixopt
Length of output: 18014
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find cluster() definition header and where tsam_kwargs is handled
rg -n "def cluster\\(" -S flixopt/transform_accessor.py
rg -n "tsam_kwargs" -S flixopt/transform_accessor.py
python - <<'PY'
from pathlib import Path
p=Path("flixopt/transform_accessor.py")
lines=p.read_text().splitlines()
# print a window around first occurrence of 'def cluster(' and around where tsam_kwargs is described/used
def show(center, span=80):
start=max(1, center-span)
end=min(len(lines), center+span)
for i in range(start, end+1):
print(f"{i}:{lines[i-1]}")
for idx,line in enumerate(lines, start=1):
if "def cluster(" in line:
print("\n--- def cluster() context ---")
show(idx, 120)
break
# find line that contains 'Additional keyword arguments passed'
for idx,line in enumerate(lines, start=1):
if "Additional keyword arguments passed to" in line:
print("\n--- tsam_kwargs doc context ---")
show(idx, 120)
break
PYRepository: flixOpt/flixopt
Length of output: 25359
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find cluster() definition header and where tsam_kwargs is handled
rg -n "def cluster\\(" -S flixopt/transform_accessor.py
rg -n "tsam_kwargs" -S flixopt/transform_accessor.py
python - <<'PY'
from pathlib import Path
p=Path("flixopt/transform_accessor.py")
lines=p.read_text().splitlines()
def show(center, span=80):
start=max(1, center-span)
end=min(len(lines), center+span)
for i in range(start, end+1):
print(f"{i}:{lines[i-1]}")
# cluster() context
for idx,line in enumerate(lines, start=1):
if "def cluster(" in line:
print("\n--- def cluster() context ---")
show(idx, 130)
break
# tsam_kwargs doc context
for idx,line in enumerate(lines, start=1):
if "Additional keyword arguments passed to ``tsam.aggregate()``" in line:
print("\n--- tsam_kwargs doc context ---")
show(idx, 130)
break
# where tsam_kwargs is likely passed in / constructed (look for **tsam_kwargs usage sites)
for idx,line in enumerate(lines, start=1):
if "**tsam_kwargs" in line:
print(f"\n--- **tsam_kwargs usage around line {idx} ---")
show(idx, 60)
PYRepository: flixOpt/flixopt
Length of output: 47349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import warnings
import numpy as np
import pandas as pd
from tsam import aggregate, ClusterConfig
# minimal time series-like DataFrame for tsam
df = pd.DataFrame(np.arange(12, dtype=float).reshape(6,2), columns=["x","y"])
base = ClusterConfig(
method="hierarchical",
representation="medoid",
weights={"x": 1.0}, # non-None to show deprecation behavior
)
# 1) Passing weights via ClusterConfig may warn (depending on tsam)
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
try:
aggregate(
df,
n_clusters=2,
period_duration=24,
temporal_resolution=None,
cluster=base,
weights=None,
)
except Exception:
# ignore validation errors from tsam internals; we only care about DeprecationWarning emission
pass
dep_names = sorted({x.category.__name__ for x in w})
print("warnings categories (cluster_config has weights):", dep_names)
# 2) weights=None in ClusterConfig + top-level weights should not warn
cfg_no_weights = ClusterConfig(
method=base.method,
representation=base.representation,
normalize_column_means=base.normalize_column_means,
use_duration_curves=base.use_duration_curves,
include_period_sums=base.include_period_sums,
solver=base.solver,
weights=None,
)
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
try:
aggregate(
df,
n_clusters=2,
period_duration=24,
temporal_resolution=None,
cluster=cfg_no_weights,
weights={"x": 1.0},
)
except Exception:
pass
dep_names = sorted({x.category.__name__ for x in w})
print("warnings categories (cluster_config weights=None):", dep_names)
PYRepository: flixOpt/flixopt
Length of output: 426
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import numpy as np
import pandas as pd
from tsam import aggregate, ClusterConfig
# Create a small hourly time series with a datetime index
idx = pd.date_range("2020-01-01", periods=48, freq="h") # 2 days
df = pd.DataFrame(
{
"x": np.sin(np.linspace(0, 10, len(idx))) + 2.0,
"y": np.cos(np.linspace(0, 10, len(idx))) + 5.0,
},
index=idx,
)
# Case A: top-level weights= accepted
cfg = ClusterConfig(method="hierarchical", representation="medoid", weights=None)
res = aggregate(
df,
n_clusters=2,
period_duration=24,
temporal_resolution="1h",
cluster=cfg,
weights={"x": 1.0, "y": 2.0},
)
print("aggregate(weights=...) succeeded:", type(res).__name__)
# Case B: ensure python doesn't crash due to duplicate weights at call site
try:
aggregate(
df,
n_clusters=2,
period_duration=24,
temporal_resolution="1h",
cluster=cfg,
weights={"x": 1.0},
**{"weights": {"x": 2.0}}, # would normally cause "multiple values" at call site
)
except TypeError as e:
print("duplicate weights via **tsam_kwargs would TypeError:", e)
PYRepository: flixOpt/flixopt
Length of output: 496
🏁 Script executed:
rg -n "^import dataclasses\\b|\\bfrom dataclasses import\\b" flixopt/transform_accessor.pyRepository: flixOpt/flixopt
Length of output: 41
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "\\bdataclasses\\b" -S flixopt | head -n 50Repository: flixOpt/flixopt
Length of output: 369
Refactor ClusterConfig reconstruction + guard tsam_kwargs from passing weights
- For
flixopt/transform_accessor.py_build_cluster_config_with_weights(933-948):tsam.ClusterConfigis a dataclass anddataclasses.replace(cluster, weights=None)does not emit aDeprecationWarningontsam==3.4.0; prefer this over manually re-listing every field for future-proofing.
♻️ Proposed refactor
+ import dataclasses
+
# No ClusterConfig provided - use defaults (weights passed to aggregate())
if cluster is None:
return ClusterConfig(), weights
- # ClusterConfig provided - preserve its settings; weights passed to aggregate()
- return (
- ClusterConfig(
- method=cluster.method,
- representation=cluster.representation,
- normalize_column_means=cluster.normalize_column_means,
- use_duration_curves=cluster.use_duration_curves,
- include_period_sums=cluster.include_period_sums,
- solver=cluster.solver,
- ),
- weights,
- )
+ # ClusterConfig provided - drop only weights, preserve every other field
+ # (incl. fields added in future tsam versions). weights=None should not
+ # trigger the deprecation warning.
+ return dataclasses.replace(cluster, weights=None), weights🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@flixopt/transform_accessor.py` around lines 933 - 948, Replace the manual
re-construction of ClusterConfig in _build_cluster_config_with_weights with
dataclasses.replace(cluster, weights=None) to avoid re-listing fields (use
ClusterConfig via dataclasses.replace). Also ensure the code that builds
tsam_kwargs/reserved_tsam_keys excludes top-level 'weights' by adding 'weights'
to reserved_tsam_keys (so weights is passed only via the explicit weights=...
arg to tsam.aggregate and not via **tsam_kwargs), preventing "multiple values
for keyword argument 'weights'".
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
1 participant
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.
Why docs stopped publishing
The docs site (
mike→gh-pages) is pinned at 6.1.0 — releases 6.1.1, 6.1.2, 6.1.3 never deployed. Thedeployjob indocs.yamlhasneeds: build, and thebuildjob has been failing at Execute fast notebooks since ~6.1.0.Root cause chain (reproduced locally):
create_district_heating_system(), which builds load profiles viademandlib.demandlibruns a globalwarnings.simplefilter("error")(bdew/elec_slp.py) and never restores it — so every subsequent warning in the kernel becomes a fatal exception.ClusterConfig(weights=...), which tsam ≥ 3.4.0 deprecates.DeprecationWarning→ raised →CellExecutionError→ notebook step fails →buildfails →deployis skipped.Changes
flixopt/transform_accessor.py— resolve clustering weights separately and pass them as the top-levelweights=argument totsam.aggregate()instead of via the deprecatedClusterConfig(weights=...). (the actual fix)docs/notebooks/data/generate_realistic_profiles.py— wrap thedemandlibcall inwarnings.catch_warnings()so its global warnings-as-error side effect can no longer leak and turn unrelated third-party warnings into hard notebook failures..github/workflows/docs.yaml— addflixopt/**to the PR path filter. The notebooksimport flixopt, but library-only PRs (like the tsam migration that introduced this) previously skipped the docs build, so the breakage only surfaced post-merge.Verification
-W error::DeprecationWarning(both default and user-providedClusterConfigpaths).08c-clustering.ipynbexecutes end-to-end: exit 0, zero cell errors (previously failed).error::Warningfilter leaks into global state after building the example system.pytest tests/test_clustering/→ 250 passed.Note: still-stale versions
mike'slatestalias is still 6.1.0. After merge, 6.1.1–6.1.3 won't auto-publish — either let the next release deploy them, or manually run the Docs workflow viaworkflow_dispatchwithdeploy: true,version: v6.1.3.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactoring
Chores