Finalise/simplify GB model names; make the trend adjuster a boolean by braddf · Pull Request #379 · openclimatefix/quartz-api

braddf · 2026-09-03T16:23:13Z

Pull Request

Description

Rename the user-facing GB forecast models to describe their input sets rather than internal model names, so they line up across products, and move the trend adjuster out of the model name into an adjusted boolean. Internal DP forecaster names are unchanged — this is a naming and parameter change only.

`model`	Inputs	Internal DP forecaster	Retired names still accepted
`blend`	blend of all models	`blend`	`blend_adjust`
`ecmwf_mo`	ECMWF + Met Office (day-ahead: NWPs only)	`pvnet_day_ahead`	`pvnet_day_ahead`, `pvnet_day_ahead_adjust`
`ecmwf_mo_sat_8h`	ECMWF + Met Office + satellite (~8h)	`pvnet_v2`	`pvnet_intraday`, `pvnet_intraday_adjust`
`ecmwf`	ECMWF only	`pvnet_ecmwf`	`pvnet_ecmwf`, `pvnet_ecmwf_adjust`
`sat_8h`	satellite only (~8h)	`pvnet_sat_only`	`pvnet_sat`, `pvnet_sat_adjust`
`mo`	Met Office only	`pvnet_ukv_only`	`pvnet_ukv`, `pvnet_ukv_adjust`

National exposes all six; GSP exposes blend, ecmwf_mo_sat_8h and ecmwf_mo.

adjusted boolean, defaulting to on — ?adjusted=true|false on /regions/{id}/forecast, /forecast/last-updated and /forecasts/snapshot. It is orthogonal to model choice and every adjustable model has an adjusted variant, so defaulting it on means callers get the best available forecast without knowing to ask. Leaves room to become a select later, or to disappear if the adjustment moves into the model pipeline.

Not every region type has adjusted variants — GB gsp and NL province have none. There the param is ignored rather than rejected: a 400 would force a caller sweeping several region types to special-case GSP. GET /region-types now returns supports_adjusted per type so that stays discoverable instead of silent.

model_name in responses no longer carries an _adjust suffix, matching v0, which stripped it before returning it too.

Scope

GB naming only. NL slugs are unchanged; NL picks up the adjusted param and the alias mechanism for free.
Not on /forecasts/period or /generation/period — those are cache-only and serve the pre-warmed default, and adding the param would mean warming two variants per region.

Backwards compatibility

The retired names in the table above are carried on ForecastModel.aliases / adjust_aliases and stay out of /region-types, the OpenAPI enum and 400 messages, so existing integrations keep working while new ones only ever see the current names. An alias pins its own adjuster state — the _adjust ones pin it on, the rest pin it off — so an old name returns byte-for-byte what it did before.

⚠️ One behaviour change. blend is both a current name and a pre-rename name, so it cannot carry an alias override: it takes the new adjusted=true default and now returns the adjusted blend where it previously returned the plain one. ?adjusted=false restores the old result. Our one external tester is on blend, so this wants a heads-up before it ships.

Also included

An intraday-only caller who omitted model could fall back to the unrestricted default — the model that naming it explicitly would have returned 403 for. Unreachable from the current config, but the fallback now stays within the intraday set, and the config invariant test requires an intraday default whenever intraday_models is set.

Checklist:

My code follows OCF's coding style guidelines
I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
I have checked my code and corrected any misspellings

Rename the user-facing GB forecast models to describe their input sets, so they line up across products. Internal DP forecaster names are unchanged. blend blend ecmwf_mo pvnet_day_ahead ecmwf_mo_sat_8h pvnet_v2 ecmwf pvnet_ecmwf sat_8h pvnet_sat_only mo pvnet_ukv_only The trend adjuster moves out of the model name and into an `adjusted` boolean, defaulting to on. Region types declare `supports_adjusted`, surfaced on GET /region-types, so a caller can tell where the param applies without trial and error; where it does not (GB gsp, NL province) it is ignored rather than rejected. Every retired slug still resolves via ForecastModel.aliases / adjust_aliases, unadvertised in /region-types, the OpenAPI enum and 400 messages. An alias pins its own adjuster state, so an existing client keeps the exact forecast it had. The one exception is `blend`, which is both a current and a pre-rename name: it takes the new default and now returns the adjusted blend. Pass adjusted=false for the old behaviour. Also closes a latent hole where an intraday-only caller who omitted `model` could fall back to the unrestricted default — the model naming it explicitly would have returned 403. Unreachable from the current config; the config invariant test now requires an intraday default whenever intraday_models is set.

peterdudfield · 2026-09-03T16:38:36Z

+    def accepts(self, api_name: str) -> bool:
+        """True if `api_name` names this model, current or legacy."""
+        return (
+            api_name == self.api_name


I think you can do api_name in [self.api_name, *self.aliases, *self.adjust_aliases]. to make it neater

Yep updated, used a tuple instead of a list so we're not building a throwaway list on every call, but same idea.

Then I actually dropped this accepts() altogether. It only had one caller, and then from tweaks re Suvan's comment, that caller always passed in the region type's own supports_adjusted flag just to get it handed back – so the check now lives in get_model_by_api_name, right next to get_model_by_internal_name, which already reads those fields anyway.

peterdudfield · 2026-09-03T16:40:55Z

What country is this for?

⚠️ One behaviour change. blend is both a current name and a pre-rename name, so it cannot carry an alias override: it takes the new adjusted=true default and now returns the adjusted blend where it previously returned the plain one. ?adjusted=false restores the old result. Our one external tester is on blend, so this wants a heads-up before it ships.

peterdudfield · 2026-09-03T16:42:36Z

Would anything in the api checks here need to change?

peterdudfield · 2026-09-03T16:43:57Z

    label: str
    slug: str | None = None
+    adjust_name: str | None = None
+    aliases: tuple[str, ...] = ()


how come you didnt make these lists of strings?

I'll admit my view of Python dataclasses rules and regs is still mixed, but I spent a bit of time thinking about this config, and there are a couple of things here trying to keep these top Classes tight as poss:

A plain list default won't actualy import – dataclasses reject it: ValueError: mutable default <class 'list'> for field aliases is not allowed: use default_factory. So every alias field would need wrapping in field(default_factory=list), which feels like a bit of shoehorning in comparison to a tuple, which does work.

Secondly, frozen=True only stops you replacing a field, not changing what's inside it. fm.aliases = (...) is blocked, but fm.aliases.append(...) would work fine on a list. COUNTRIES is built once when the module loads and shared by every request, so a stray append anywhere would change model routing for everything that worker handles afterwards... and only that worker, which would be horrible to debug. A tuple has no append, so it can't happen by design, so I'd say this is the pattern we want to adopt

Thanks, that sounds sensible

peterdudfield

Ive approved,

Ive add a few questions, but nothing major

suvanbanerjee · 2026-09-04T05:08:20Z

    location_type = locs[0].location_type or models.LocationType.NATION
    rt = country.location_type_to_region_type(location_type)
-    model = resolve_forecast_model(model, rt, is_intraday_only)
+    model = resolve_forecast_model(model, rt, is_intraday_only, adjusted)


I think before this add validate_model(model_name, rt, rt.type)

Agreed, added. It was the only forecast route not validating, potentially hitting the DP instead of returning 400, must have missed that one!

suvanbanerjee · 2026-09-04T05:28:34Z

+        return (
+            api_name == self.api_name
+            or api_name in self.aliases
+            or api_name in self.adjust_aliases


I think here if someone tries to get GSP with blend_adjust this will allow it, check this behavior against the old object check once

Amended! Now checks for these "non-existent" regional models if it's an adjust_alias and returns false if they shouldn't be allowed. Good catch 👍

suvanbanerjee

Couple of comments from my side but overall looks solid!

Reject `_adjust` aliases where no adjusted variant exists. GB gsp and NL province have none, so `?model=blend_adjust` was matching the alias and then resolving to plain `blend` — accepting the name and quietly serving something else. It 400s again, as it does on main. Validate the model on /forecast/last-updated, the one forecast route that passed an unrequested name straight to the DP. Also fold the three-way `or` in ForecastModel.accepts into a tuple membership test.

The method had one caller, which always passed the region type's own supports_adjusted and got it handed straight back — a boolean parameter carrying no information the caller didn't already have. Matching now happens where that policy lives, alongside get_model_by_internal_name, which already reads the same fields directly.

braddf · 2026-09-04T09:17:35Z

What country is this for?

⚠️ One behaviour change. blend is both a current name and a pre-rename name, so it cannot carry an alias override: it takes the new adjusted=true default and now returns the adjusted blend where it previously returned the plain one. ?adjusted=false restores the old result. Our one external tester is on blend, so this wants a heads-up before it ships.

Both GB and NL national – thanks, the description said just GB but this does apply to NL going forwards too, even if it's backward compatible for both countries for other model names. ?model=blend resolved to nl_blend before and nl_blend_adjust now, matching new GB behaviour.
NL province and GB gsp stay the same, as neither has adjusted variants nor were these allowed before with the _adjust suffix. One other thing I've just realised re NL naming, hopefully the last thing to finalise: ecmwf_mo_sat_uncurtailed isn't being renamed, so that one does change behaviour alongside the blend, but I wonder if we should actually rename this one anyway to blend_uncurtailed, unless that's technically not accurate..?

braddf · 2026-09-04T09:42:19Z

Would anything in the api checks here need to change?

Had a look – nothing needs to change. check-nl-api.py never passes a model: it's left as None at both check_forecast call sites, and check_forecast_at_timestamp uses province, whose default is unchanged as well 👍

Separate thing I noticed while I was in there, in the DAGs repo rather than this one – check-nl-api.py line 70 adds model_name=... to the per-region forecast URL, but that route's parameter is called model (model_name only exists on /forecasts/snapshot, and that's annoying so we should line it up to match the rest of the routes too – I have a separate issue to go through and make sure we fix all these tiny inconsistencies). Nothing passes it at the mo, so it's harmless so far, but anyone who adds a check with that will get it silently ignored and probably be confused!

braddf · 2026-09-04T09:42:46Z

cc. @peterdudfield in case you miss these comments ☝️

peterdudfield · 2026-09-04T09:49:23Z

Would anything in the api checks here need to change?

Had a look – nothing needs to change. check-nl-api.py never passes a model: it's left as None at both check_forecast call sites, and check_forecast_at_timestamp uses province, whose default is unchanged as well 👍

Separate thing I noticed while I was in there, in the DAGs repo rather than this one – check-nl-api.py line 70 adds model_name=... to the per-region forecast URL, but that route's parameter is called model (model_name only exists on /forecasts/snapshot, and that's annoying so we should line it up to match the rest of the routes too – I have a separate issue to go through and make sure we fix all these tiny inconsistencies). Nothing passes it at the mo, so it's harmless so far, but anyone who adds a check with that will get it silently ignored and probably be confused!

Sounds good, do you mind adding the link of that issue here, just so it links back to this comment?

braddf · 2026-09-04T10:11:31Z

@peterdudfield added link to my reply and also added issue for check here: https://github.com/openclimatefix/airflow-dags/issues/806

peterdudfield · 2026-09-04T11:28:26Z

-        label="PV + ECMWF + Met Office + Satellite, Uncurtailed (Trend Adjusted)",
-        slug="ecmwf_mo_sat_uncurtailed_adjust",
+        adjust_name="nl_regional_pv_ecmwf_mo_sat_uncurtailed_adjust",
+        adjust_aliases=("ecmwf_mo_sat_uncurtailed_adjust",),


change slug to ="pv_ecmwf_mo_sat_uncurtailed_adjust"

aliases = "ecmwf_mo_sat_uncurtailed",

The slug and the alias were the wrong way round: the old name `ecmwf_mo_sat_uncurtailed` was still advertised and still picked up the new adjusted-by-default, while the new PV-inclusive name was hidden as a pinned alias no client had ever used. Swapped, so `ecmwf_mo_pv_sat_uncurtailed` is the advertised name and the old one is a pinned alias returning exactly what it did on main. `blend` is now the only name whose behaviour changes in this PR.

braddf requested review from peterdudfield and suvanbanerjee September 3, 2026 16:24

braddf self-assigned this Sep 3, 2026

peterdudfield reviewed Sep 3, 2026

View reviewed changes

peterdudfield approved these changes Sep 3, 2026

View reviewed changes

suvanbanerjee reviewed Sep 4, 2026

View reviewed changes

braddf added 2 commits September 4, 2026 09:26

suvanbanerjee approved these changes Sep 4, 2026

View reviewed changes

peterdudfield reviewed Sep 4, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

braddf commented Sep 3, 2026

Pull Request

Description

Backwards compatibility

Also included

Checklist:

Uh oh!

peterdudfield Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterdudfield commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peterdudfield commented Sep 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterdudfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suvanbanerjee Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suvanbanerjee left a comment

Choose a reason for hiding this comment

Uh oh!

braddf commented Sep 4, 2026

Uh oh!

braddf commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

braddf commented Sep 4, 2026

Uh oh!

peterdudfield commented Sep 4, 2026

Uh oh!

braddf commented Sep 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peterdudfield Sep 3, 2026 •

edited

Loading

peterdudfield commented Sep 3, 2026 •

edited

Loading

suvanbanerjee Sep 4, 2026 •

edited

Loading

braddf commented Sep 4, 2026 •

edited

Loading