Capture optimizer ESDLs as CI artifacts to update ESDL references

[jarsarasty]

During each grow optimizer test run, the actual optimizer output ESDL is now written to a separate artifacts directory (distinct from the reference output/ directory, so references are never overwritten by tests). This directory is mounted as a Docker volume and uploaded as a GitHub Actions artifact on every push, even when tests fail.

This PR also updates the 5 grow optimizer ESDL reference snapshots to match optimizer-worker 2.1.10 output. Changes vs previous references:

• dataSource elements added to all profile nodes (optimizer provenance tagging)
• Pipe sizing changed DN200 → DN150
• PostProc.Pressure profiles removed from pipes
• Power and investment cost values updated accordingly

CI is passing.

[cwang39403]

When the CI run fails due to a discrepancy between the result ESDLs and the reference ESDLs, I think it would be helpful to document and emphasize the need for expert review of the output ESDLs in the /system_tests/README. With the overall flow as below.

CI run fails (compare_esdl diff)
        ↓
Download artifact from GitHub Actions
        ↓
Expert reviews the ESDL diff (is this change physically correct?)
        ↓
Copies artifact → test_esdl/output/<test_name>.esdl
        ↓
Commits + opens PR
        ↓
PR review (second pair of eyes)
        ↓
Merge → new reference baseline

Now we are assuming the mesido results from v0.1.20 (optimizer-worker 2.1.10) are correct and simply overwrite those output ESDLs used by test, so the CI can pass, but looking at this difference, I am not 100% sure if this is intentional to replace the KPI label from asset name with UUID. This is just an example.

[cwang39403]

Minor: OPTIMIZER_ARTIFACTS_DIR=/app/test_esdl/artifacts should be added to .env.template?

[cwang39403]

@jarsarasty, you can merge this MR whenever suits you.

@MarkTNO, just to keep you updated about these changes.

[MarkTNO]

Thanks for the work, nice addition to have the test output ESDL's available as artifact.

I would not want to merge this PR yet, but first check with the updates to the output ESDL's with @KobusVanRooyen. Are those different prediction ok?

[jarsarasty]

I am not 100% sure if this is intentional to replace the KPI label from asset name with UUID. This is just an example.

Hi @cwang39403 and @MarkTNO, Thanks for your comments.

@KobusVanRooyen: In write_output, line 473 changed from indexing heat_source_energy_wh by asset.name to indexing it by asset_id. This change happened in this commit. If this change was not intentional, we could use something like:

heat_source_energy_wh[f"{asset.name} [{asset_id}]"]

[KobusVanRooyen]

I am not 100% sure if this is intentional to replace the KPI label from asset name with UUID. This is just an example.

Hi @cwang39403 and @MarkTNO, Thanks for your comments.

@KobusVanRooyen: In write_output, line 473 changed from indexing heat_source_energy_wh by asset.name to indexing it by asset_id. This change happened in this commit. If this change was not intentional, we could use something like:

heat_source_energy_wh[f"{asset.name} [{asset_id}]"]

This change was intentional, since we are not using asset name anymore to refer to an asset. So using that is why the asset id is also used in heat_source_energy_wh, and therefor it is seen in the kpi label

[KobusVanRooyen]

I am not 100% sure if this is intentional to replace the KPI label from asset name with UUID. This is just an example.

Hi @cwang39403 and @MarkTNO, Thanks for your comments.
@KobusVanRooyen: In write_output, line 473 changed from indexing heat_source_energy_wh by asset.name to indexing it by asset_id. This change happened in this commit. If this change was not intentional, we could use something like:
heat_source_energy_wh[f"{asset.name} [{asset_id}]"]

This change was intentional, since we are not using asset name anymore to refer to an asset. So using that is why the asset id is also used in heat_source_energy_wh, and therefor it is seen in the kpi label

After a discussion with Cheng-Kai I have realized the challenge with this update. MESIDO will be updated temporarily to use the name again. The we have time to come up with a better solution afterwards.

[jarsarasty]

After a discussion with Cheng-Kai I have realized the challenge with this update. MESIDO will be updated temporarily to use the name again. The we have time to come up with a better solution afterwards.

Since asset names are not unique, and to keep KPI labels human-readable, a stable label could be built in write_output: using the asset name as-is when unique, or appending a deterministic numeric suffix (sorted by asset_id) when names collide (e.g. Pump_1, Pump_2). This avoids the revert and keeps labels stable across runs. @KobusVanRooyen: if you agree with this approach, I can implement it next week.

[KobusVanRooyen]

After a discussion with Cheng-Kai I have realized the challenge with this update. MESIDO will be updated temporarily to use the name again. The we have time to come up with a better solution afterwards.

Since asset names are not unique, and to keep KPI labels human-readable, a stable label could be built in write_output: using the asset name as-is when unique, or appending a deterministic numeric suffix (sorted by asset_id) when names collide (e.g. Pump_1, Pump_2). This avoids the revert and keeps labels stable across runs. @KobusVanRooyen: if you agree with this approach, I can implement it next week.

@jarsarasty no I do not like this idea 100%, but thanks for the offer. For now we will implement a temporary solution while we try to find a better solution.

[MarkTNO]

Seeing the comment Kobus this PR is ok to merge.
But there should be a 'temporary solution' like @KobusVanRooyen mentioned. Now the ID will be shown in some plots instead of the asset name.

[jarsarasty]

Rebased and merged.
Thank you all for the feedback.