feat: update apispec for polymorphic resource class definition by SalimKayal · Pull Request #1388 · SwissDataScienceCenter/renku-data-services

SalimKayal · 2026-07-15T13:51:12Z

Summary

Adds a kind and optional FirecREST remote override to resource classes so that FirecREST-backed
classes can target specific Slurm systems/partitions and optionally let the HPC grid choose CPU,
memory, and GPU. A single resource pool can now contain classes with different Slurm target
configurations.

Motivation and context

We originally targeted only CSCS, where the smallest allocation is a full node, so it was enough to
to let Slurm set the defaults for CPUS_PER_TASK, GPUS, RAM. As we expand to other grids
(Idiap) we need to tell Amalthea exactly how much of a node to request. We also want a single
resource pool to support multiple class-level target configurations and optionally which
system_name/partition to use, rather than forcing one pool per Slurm target.

Behavior

Resource classes now have a kind (local, firecrest, runai). The kind must match the parent
pool's remote kind; firecrest classes require integer CPU and may only carry remote overrides
when the pool is FirecREST.
FirecREST classes support per-class remote overrides for system_name and partition, falling
back to the pool defaults when omitted. This lets a single pool hold multiple classes targeting
different Slurm systems or partitions.
FirecREST classes can set ignore_resource_class_values to tell Amalthea to ignore the CRD CPU,
memory, and GPU values and let the HPC grid choose them. The class still stores and reports the
values for display and quota matching.
On session launch, FirecREST classes emit RSC_FIRECREST_SYSTEM_NAME, RSC_FIRECREST_PARTITION
(when set), and RSC_FIRECREST_IGNORE_RESOURCE_CLASS_VALUES (when true). CPU, memory, and GPU
continue to be passed through the session CRD resources as before.
Changing a pool's remote kind auto-converts existing classes to the new kind. The update is
rejected if any class still has FirecREST remote overrides when moving away from FirecREST, or if
any class has non-integer CPU when moving to FirecREST.
The API remains backward compatible: resource class schemas default kind to local, so existing
clients and legacy classes keep working.

Changes

Updated crc/api.spec.yaml and regenerated crc/apispec.py:
- ResourceClass/ResourceClassWithId now include kind: RemoteKind (default local) and an
  optional remote: RemoteClassConfigurationFirecrest.
- RemoteClassConfigurationFirecrest gains ignore_resource_class_values (default false).
- Added if/then/else schema rules so remote is forbidden when kind != firecrest and cpu
  must be an integer when kind == firecrest.
Extended crc/models.py with FirecrestClassRemote and added kind/remote to
UnsavedResourceClass, ResourceClass, and ResourceClassPatch.
Extended crc/orm.py with a remote_json column for class-level FirecREST overrides. Class
kind is derived at read time from the parent pool's remote_json.kind.
Added an Alembic migration that adds resource_classes.remote_json.
Updated crc/core.py validators to:
- Enforce pool-kind/class-kind matching.
- Require integer CPU for FirecREST classes.
- Reject remote overrides on non-FirecREST classes.
- Derive kind correctly on PUT/PATCH when omitted.
- Validate class compatibility when a pool's remote kind changes.
Updated crc/db.py to persist remote_json, reject kind changes, and apply pool remote changes
before class updates so class conversion sees the new pool kind.
Updated crc/blueprints.py to pass the existing class kind into standalone class PUT/PATCH
validators.
Updated notebooks/core_sessions.py to compute effective FirecREST env vars from class overrides
and emit RSC_FIRECREST_IGNORE_RESOURCE_CLASS_VALUES when requested.
Added unit tests for the new validators and updated FirecREST resource-pool and session-launch
tests.

Notes

Resource class kind is not stored on the resource_classes table; it is derived at read time
from the parent pool's remote_json.kind (defaulting to local). This keeps the migration
minimal (only remote_json is added) while still making existing FirecREST/Run:AI classes appear
with the correct kind.
The UI can still read/write pool-level system_name/partition for now; they serve as the
defaults for classes without per-class overrides.

coveralls · 2026-08-04T12:32:35Z

Coverage Report for CI Build 31506944335

Coverage at 85.878% (no base build to compare)

Details

Coverage remained the same as the base build.
Patch coverage: 8 uncovered changes across 4 files (129 of 137 lines covered, 94.16%).
No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
components/renku_data_services/crc/db.py	22	18	81.82%
components/renku_data_services/crc/core.py	47	45	95.74%
components/renku_data_services/crc/models.py	18	17	94.44%
components/renku_data_services/notebooks/core_sessions.py	15	14	93.33%
Total (8 files)	137	129	94.16%

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	32410
Covered Lines:	27833
Line Coverage:	85.88%
Coverage Strength:	1.49 hits per line

💛 - Coveralls

leafty · 2026-08-05T13:03:34Z

          $ref: "#/components/schemas/NodeAffinityList"
        quota_enforced:
          $ref: "#/components/schemas/QuotaEnforced"
+        kind:


I don't understand why we repeat kind in the resource class when it cannot be different from the parent resource pool.

it allows the conditional in the api spec. I hesitated a lot with this because datamodel code generator does not support this. I can change that if you think it's more trouble than otherwise

kind is gone, per #1404

leafty · 2026-08-05T13:10:53Z

+      then:
+        properties:
+          cpu:
+            $ref: "#/components/schemas/CpuInteger"


Why do we add this complexity in the API spec? I would rather have this handled by a math.floor() inside the remote session controller on the amalthea side than make a complex API spec.

because floor of 0.5 is not what we want ? but again this can be validated elsewhere (i.e. in core.py)

I mean, we can have:

cpu_int = max(math.floor(cpu), 1)

to have a minimum of 1 and then truncating if set to a non-whole value

out of the api spec per #1404

SalimKayal

So basically all your remarks are linked to the kind in resource class. should I remove this and manage this aspect differently ?

SalimKayal · 2026-08-05T14:09:38Z

          $ref: "#/components/schemas/NodeAffinityList"
        quota_enforced:
          $ref: "#/components/schemas/QuotaEnforced"
+        kind:


it allows the conditional in the api spec. I hesitated a lot with this because datamodel code generator does not support this. I can change that if you think it's more trouble than otherwise

SalimKayal · 2026-08-05T14:10:52Z

+      then:
+        properties:
+          cpu:
+            $ref: "#/components/schemas/CpuInteger"


because floor of 0.5 is not what we want ? but again this can be validated elsewhere (i.e. in core.py)

leafty · 2026-08-10T12:49:08Z

+          $ref: "#/components/schemas/RemoteConfigurationFirecrestSystemName"
+        partition:
+          $ref: "#/components/schemas/RemoteConfigurationFirecrestPartition"
+        ignore_resource_class_values:


Would it make more sense to move this field to the resource class? It seems that all the classes in a remote HPC pool would set this to the same value.

I wondered a bit about this. Here's why I went this way.

If we keep the resource pools/classes as is it works because the resource pool's values are used.

if we change the values in the resource class, it overrides the pool's defaults

this means that if we do not change anything in the configs it should still work, if we do we can have a single resource pool per firecrest api endpoint. i.e. CSCS Alps with Daint/Eiger/Bristen/Clariden Systems and debug/normal partitions. I can change that if you think it's poor design.

I think it would be more likely that an admin forgets about this field when updating some classes. If we have forward_resource_values at the resource pool level, then it is less likely to be mis-configured (only needs to be set once per pool).

the default for the forward_resource_values is false. we may want to have some classes with defaults and some with config. Also we could have a default in the pool and an override in the class ? Or do we disregard this feature for admins and simplify it at the pool level ?

Is there a case where a resource pool would have a mix bag of forward: true and forward: false for classes? It seems to me that the setting could be left at the pool level.

At CSCS they were talking about allowing partial node allocation because some people were complaining that 4xGB200 is a bit much for some tasks.

leafty

Comment: without a platform override (amd64 | arm64), it is not possible to merge clusters in a single resource pool.

SalimKayal · 2026-08-11T13:32:20Z

Comment: without a platform override (amd64 | arm64), it is not possible to merge clusters in a single resource pool.

Should I add it while I'm at it ?

leafty · 2026-08-11T13:36:39Z

Comment: without a platform override (amd64 | arm64), it is not possible to merge clusters in a single resource pool.

Should I add it while I'm at it ?

Sure.

SalimKayal · 2026-08-11T13:48:13Z

Comment: without a platform override (amd64 | arm64), it is not possible to merge clusters in a single resource pool.

Should I add it while I'm at it ?

Sure.

this is a bigger redesign as we need to use it to validate the image builds. do I simply wire it in for now then add the ui/image-building stuff in a follow up PR or is all of this a follow up pr ?

leafty · 2026-08-11T14:24:57Z

Comment: without a platform override (amd64 | arm64), it is not possible to merge clusters in a single resource pool.

Should I add it while I'm at it ?

Sure.

this is a bigger redesign as we need to use it to validate the image builds. do I simply wire it in for now then add the ui/image-building stuff in a follow up PR or is all of this a follow up pr ?

We should leave it out then.

SalimKayal · 2026-08-11T14:38:42Z

Info: ignore_resource_class_values renamed to forward_resource_value

…lity

…epository

…g kind

* feat: remove redundant kind field from resource-class API spec * refactor: derive resource class local/remote from parent pool * fix: components/renku_data_services/crc/apispec.py * test: update resource pool API tests for removed kind field * refactor: reset QuotaEnforced defaults at apispec level

…_kind param

…RD_RESOURCE_VALUES

…n API spec

…n model + validators

… sessions

leafty

Looks good now, we should deploy and test.

leafty · 2026-08-12T08:44:14Z

+          $ref: "#/components/schemas/RemoteConfigurationFirecrestSystemName"
+        partition:
+          $ref: "#/components/schemas/RemoteConfigurationFirecrestPartition"
+        forward_resource_value:


Suggested change

forward_resource_value:

forward_resource_values:

leafty · 2026-08-12T08:45:46Z

+        # FirecREST requires integer CPU.
+        cpu = rc.cpu if rc.cpu is not None else (existing.cpu if existing is not None else None)
+        if new_pool_kind == models.RemoteConfigurationKind.firecrest and cpu is not None and not cpu.is_integer():
+            raise errors.ValidationError(message="FirecREST resource classes require an integer value for cpu.")


Can we not do ceil() instead? This could be annoying to deal with. (Or the admin would have to delete and re-create).

The check only fires on writes (PUT/PATCH of the pool, POST of a class). GETs don't validate, so existing fractional-cpu classes still read back fine and don't need migrating.

Failing at write time is actually the right moment to be explicit: the admin is already making a change, so a clear error tells them to set an integer cpu in the same request rather than silently coercing a value they typed.

For the case where fractional data does slip through (legacy rows, etc.), I've switched the launch-time coercion in core_sessions.py from int() (truncated down) to ceil() (rounds up). That achieves the "never under-provision" result without hiding the write-time mismatch behind a silent fix.

Co-authored-by: Flora Thiebaut <flora@leafty.dev>

SalimKayal force-pushed the salimkayal/feat/expend-resource-classes-for-remote-sessions branch from 7dbc2f8 to d76ebb5 Compare August 4, 2026 09:49

SalimKayal force-pushed the salimkayal/feat/expend-resource-class-apispec-for-remote-sessions branch from 225f912 to 0646568 Compare August 4, 2026 10:11

SalimKayal force-pushed the salimkayal/feat/expend-resource-class-apispec-for-remote-sessions branch 2 times, most recently from df18a6a to 964aeaf Compare August 4, 2026 14:33

SalimKayal marked this pull request as ready for review August 5, 2026 09:34

SalimKayal requested review from a team and sgaist as code owners August 5, 2026 09:34

SalimKayal requested review from leafty and olevski August 5, 2026 09:35

SalimKayal changed the base branch from salimkayal/feat/expend-resource-classes-for-remote-sessions to main August 5, 2026 09:41

leafty reviewed Aug 5, 2026

View reviewed changes

SalimKayal commented Aug 5, 2026

View reviewed changes

SalimKayal mentioned this pull request Aug 6, 2026

refactor: remove kind from resource class apispec #1404

Merged

SalimKayal requested a review from leafty August 7, 2026 15:30

leafty reviewed Aug 10, 2026

View reviewed changes

leafty reviewed Aug 11, 2026

View reviewed changes

SalimKayal added 7 commits August 11, 2026 17:11

chore: create feature branch

7d17424

feat: update apispec for polymorphic resource class definition

7d89786

feat: add kind ard remote to resource class domain models

c857b41

feat: add remote_json column for class level firecrest overrides

4789321

feat: validate class kind against pool kind and firecrest cpu integra…

6a40d36

…lity

feat: persist resource class remote_json and enforce pool kind in r…

0cf234e

…epository

feat: emit firecrest env vars from resource class

74e4a83

SalimKayal and others added 22 commits August 11, 2026 17:11

refactor: drop redundant kind-mapping helpers in hypothesis strategies

6fb09b6

test: narrow strategy in resource class put validator test

32816f2

test: remove duplicated firecrest remote env assertions

0c363b2

fix: styling

6f0269f

fix: derive kind for PUT/PATCH when omitted and guard unknown existin…

04a6d9a

…g kind

fix: pass existing class kind to standalone class PUT/PATCH validators

5c7acf9

fix: validate and convert classes when pool remote kind changes

eab5704

refactor: detangle logic

fff7a67

fix: preserve existing_kind on standalone resource-class PUT

c939fdd

refactor: remove dead kind-change guard in validate_resource_pool_update

65eaf27

refactor: pass new_pool_kind to inner class validator, drop replace()

27ceb8c

fix: clear stale class remote_json on pool kind transition + add pool…

59eac0c

…_kind param

refactor: stop setting kind on resource-class validator outputs

04e99f7

refactor: standalone class PUT/PATCH loads pool for kind derivation

d89c8ea

refactor: remove kind from all resource-class models

e5ab970

fix: stale docstrings, thin tests, deduplicate

bd9d7ee

test: expect forward_resource_value + always-emit RSC_FIRECREST_FORWA…

4007c56

…RD_RESOURCE_VALUES

feat: rename ignore_resource_class_values to forward_resource_value i…

c6adcaf

…n API spec

feat: rename ignore_resource_class_values to forward_resource_value i…

be06f79

…n model + validators

feat: always-emit RSC_FIRECREST_FORWARD_RESOURCE_VALUES for FirecREST…

e601fd5

… sessions

chore: fix migration head

0464e41

SalimKayal force-pushed the salimkayal/feat/expend-resource-class-apispec-for-remote-sessions branch from b5fcd65 to 0464e41 Compare August 11, 2026 15:25

chore: cosmetic change

fd892e9

leafty reviewed Aug 12, 2026

View reviewed changes

refactor: rename forward_resource_value to forward_resource_values

ff8faec

Co-authored-by: Flora Thiebaut <flora@leafty.dev>

leafty previously approved these changes Aug 12, 2026

View reviewed changes

fix: ceil values at session dispatch for FirecREST resource classes

8b0da36

SalimKayal dismissed leafty’s stale review via 8b0da36 August 12, 2026 09:57

leafty approved these changes Aug 13, 2026

View reviewed changes

Uh oh!

Conversation

SalimKayal commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation and context

Behavior

Changes

Notes

Uh oh!

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

Uh oh!

Coverage Report for CI Build 31506944335

Coverage at 85.878% (no base build to compare)

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

leafty Aug 5, 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!

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!

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!

Uh oh!

SalimKayal 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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

leafty left a comment

Choose a reason for hiding this comment

Uh oh!

SalimKayal commented Aug 11, 2026

Uh oh!

leafty commented Aug 11, 2026

Uh oh!

SalimKayal commented Aug 11, 2026

Uh oh!

leafty commented Aug 11, 2026

Uh oh!

SalimKayal commented Aug 11, 2026

Uh oh!

leafty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SalimKayal commented Jul 15, 2026 •

edited

Loading

coveralls commented Aug 4, 2026 •

edited

Loading

leafty Aug 5, 2026 •

edited

Loading