Skip to content

Add domain-size checks on FrozenStencil#239

Merged
twicki merged 20 commits into
developfrom
domain_size_checks
Oct 23, 2025
Merged

Add domain-size checks on FrozenStencil#239
twicki merged 20 commits into
developfrom
domain_size_checks

Conversation

@twicki
Copy link
Copy Markdown
Collaborator

@twicki twicki commented Sep 23, 2025

Description

Since our Quantity-Factory generally allocates more memory than strictly needed (for gt4py-compatibility as well as memory alignment) as can be seen here, if a Quantity's expected domain-size is too small for the execution domain of a stencil, it can be that we are not warned properly.

This PR adds safe-guards around these calls to make sure we are not reading from memory that is technically allocated but never properly used.

How has this been tested?

New test has been added

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included

Comment thread ndsl/dsl/stencil.py Outdated
@FlorianDeconinck
Copy link
Copy Markdown
Collaborator

Good to see this, we need to broadcast it. This might catch a ton of people off guard. I have seen things with K intervals/domains...

Comment thread ndsl/dsl/stencil.py Outdated
@FlorianDeconinck
Copy link
Copy Markdown
Collaborator

So there's a main problem here: translate test are not quantity - and interface from GEOS might not be either.

A second thing, once we have something stable, shouldn't we deactivate the validate_args of gt4py?

Copy link
Copy Markdown
Collaborator

@romanc romanc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good now 👍

Copy link
Copy Markdown
Collaborator

@FlorianDeconinck FlorianDeconinck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get a one time warning flagging that some stencils are not using Quantity and therefore won't validate there sizing properly - I want to capture people varied use case before we force "Quantities everywhere" (TM)

@FlorianDeconinck FlorianDeconinck added High Priority Extra attention is needed Scheduled for next Release To be merged before the next release labels Oct 13, 2025
@FlorianDeconinck
Copy link
Copy Markdown
Collaborator

This is getting stale - let's get that warning in @twicki and push it in, I want this in the next release

Copy link
Copy Markdown
Collaborator

@FlorianDeconinck FlorianDeconinck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Let's make sure we capture a thorough ticket for the rest of the work and link it here. Top of my head:

  • clean up FV3 and/or option to run strict and fail instead of warn
  • extract extent analysis from gt4py

@twicki twicki added this pull request to the merge queue Oct 23, 2025
Merged via the queue into develop with commit f0ac6c5 Oct 23, 2025
6 checks passed
@romanc romanc deleted the domain_size_checks branch October 27, 2025 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

High Priority Extra attention is needed Scheduled for next Release To be merged before the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

out of bounds read in vertical dimesion not caught

3 participants