Feature/prepare mars2grib for cache by MircoValentiniECMWF · Pull Request #190 · ecmwf/metkit

MircoValentiniECMWF · 2026-03-29T00:48:46Z

Description

Summary

This draft PR introduces a first staged-encoding path for mars2grib, together with the minimal API exposure needed to exercise it from the current public interface.

The main goal is not to finalize the cache design yet, but to prepare the codebase for the cache mechanism that will be implemented shortly and, in the meantime, enable comparison against the existing well-tested cache-based workflow.

What is included

`SpecializedEncoder`

Added two new staged methods:

prepare(...)
finaliseEncoding(...)

These split the current hot-path header generation into:

a preparation phase, producing an immutable prepared sample
a finalisation phase, cloning the prepared sample and applying only the runtime stage

The existing encode(...) path remains unchanged.

`CoreOperations`

Added a first staged workflow on top of the encoder:

CoreOperations::CacheEntry
CoreOperations::prepare(...)
CoreOperations::finaliseEncoding(...)

This path mirrors the existing normalization / layout resolution / header encoding / value injection flow, but exposes the intermediate prepared cache object explicitly.

This is intended as a transitional step toward a future implementation where cache lifecycle and reuse will be fully internalized inside the core layer.

`Mars2Grib` API

Added temporary public exposure of the staged path:

opaque CacheEntry
prepare(...)
finaliseEncoding(...)

The public API remains specialized on:

eckit::LocalConfiguration
Options
metkit::codes::CodesHandle

The public cache object is only a thin opaque wrapper around the current core cache entry.

Important note

This PR does not claim that the staged cache API is final.

The newly exposed staged functions are temporary and are only here to:

reuse the old, already tested cache-oriented logic,
run performance comparisons,
prepare the refactoring that will soon move the full cache management inside the core layer.

So the intention of this PR is mainly:

architectural preparation,
controlled incremental migration,
benchmarking support.

Non-goals

This PR does not aim to:

finalize the public staged API,
freeze cache semantics,
complete the future internal cache implementation.

Expected next step

A follow-up step should replace this temporary staged exposure with a cleaner internal cache design managed entirely in CoreOperations, while preserving or simplifying the public API accordingly.

Remarks

A few naming inconsistencies remain in the code (par / misc) because this terminology has changed multiple times during the design discussions and is already spread across the existing codebase. I preferred not to do a large naming cleanup in this PR in order to keep the change focused on the staged/cache preparation itself.

Contributor Declaration

By opening this pull request, I affirm the following:

All authors agree to the Contributor License Agreement.
The code follows the project's coding standards.
I have performed self-review and added comments where needed.
I have added or updated tests to verify that my changes are effective and functional.
I have run all existing tests and confirmed they pass.

codecov-commenter · 2026-03-29T00:54:11Z

Codecov Report

❌ Patch coverage is 2.53165% with 77 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.94%. Comparing base (03c7e0f) to head (7a69124).

Files with missing lines	Patch %	Lines
...kit/mars2grib/frontend/header/SpecializedEncoder.h	0.00%	27 Missing ⚠️
src/metkit/mars2grib/CoreOperations.h	7.14%	26 Missing ⚠️
src/metkit/mars2grib/api/Mars2Grib.cc	0.00%	24 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #190      +/-   ##
===========================================
- Coverage    62.33%   61.94%   -0.39%     
===========================================
  Files          303      303              
  Lines        11601    11674      +73     
  Branches      1036     1046      +10     
===========================================
  Hits          7232     7232              
- Misses        4369     4442      +73

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

Prepares mars2grib for an upcoming cache mechanism by introducing a transitional staged-encoding workflow (prepare/finalise) alongside the existing encode() hot path.

Changes:

Added staged header encoding to SpecializedEncoder (prepare() + finaliseEncoding()).
Added a transitional staged cache entry and staged workflow in CoreOperations.
Exposed a temporary opaque staged cache API in Mars2Grib (CacheEntry, prepare(), finaliseEncoding()).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`src/metkit/mars2grib/frontend/header/SpecializedEncoder.h`	Adds staged encoder methods to split header encoding into preparation and runtime finalisation.
`src/metkit/mars2grib/api/Mars2Grib.h`	Exposes a temporary public staged cache API (opaque cache entry + prepare/finalise).
`src/metkit/mars2grib/api/Mars2Grib.cc`	Implements the opaque cache wrapper/deleter and forwards staged calls to `CoreOperations`.
`src/metkit/mars2grib/CoreOperations.h`	Introduces `CoreOperations::CacheEntry` plus staged `prepare()`/`finaliseEncoding()` pipeline functions.
`.github/copilot-instructions.md`	Adds repository-level Copilot instructions (doc sync checks for mars2grib-related changes).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-29T00:55:09Z

src/metkit/mars2grib/api/Mars2Grib.h

+    CacheEntryPtr prepare(const eckit::LocalConfiguration& mars, const eckit::LocalConfiguration& misc);
+


New public staged-encoding API (prepare() / finaliseEncoding()) is introduced without corresponding test coverage. There are existing mars2grib API tests (e.g. tests/mars2grib/api/test_cpp_api.cc) covering encode(), so please add at least one test that exercises prepare() + repeated finaliseEncoding() and verifies it produces a valid CodesHandle (and ideally matches encode() output for the same inputs).

It is a temporary solution that will be removed soon

src/metkit/mars2grib/frontend/header/SpecializedEncoder.h

src/metkit/mars2grib/api/Mars2Grib.cc

src/metkit/mars2grib/CoreOperations.h

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/metkit/mars2grib/frontend/header/SpecializedEncoder.h

Copilot · 2026-03-29T12:50:00Z

src/metkit/mars2grib/frontend/header/SpecializedEncoder.h

+            // @todo eventually need to return another clone to commit modifications
+            return samplePtr;


finaliseEncoding() currently returns the handle after applying the stage callbacks without performing the final stage clone that encode() does (it clones after every stage). If the clone step is required to match encode() semantics (and the comment suggests it is), clone before returning and drop the @todo so staged output is consistent with the non-staged path.

Suggested change

// @todo eventually need to return another clone to commit modifications

return samplePtr;

auto resultPtr = clone_or_throw<OutDict_t>(*samplePtr);

return resultPtr;

Potentially heavy operation in the hot path. I'll add this only if strictly needed

src/metkit/mars2grib/api/Mars2Grib.cc

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-29T13:17:36Z

src/metkit/mars2grib/api/Mars2Grib.cc

+    try {
+        return CoreOperations::finaliseEncoding<double, eckit::LocalConfiguration, eckit::LocalConfiguration, Options,
+                                                metkit::codes::CodesHandle>(
+            *(cacheEntry->impl_), Span<const double>{values}, mars, misc, opts_, language_);
+    }
+    catch (...) {
+        throw eckit::Exception("Unable to finalise encoding", Here());
+    }


The catch-all here rethrows a new eckit::Exception and discards the detailed nested exception chain produced by CoreOperations::finaliseEncoding(...). This makes debugging failures much harder and changes the observable error behavior compared to encode(). Prefer either letting the original exception propagate, or rethrowing while preserving the nested exception (e.g., std::throw_with_nested(eckit::Exception(...))).

Suggested change

try {

return CoreOperations::finaliseEncoding<double, eckit::LocalConfiguration, eckit::LocalConfiguration, Options,

metkit::codes::CodesHandle>(

*(cacheEntry->impl_), Span<const double>{values}, mars, misc, opts_, language_);

}

catch (...) {

throw eckit::Exception("Unable to finalise encoding", Here());

}

return CoreOperations::finaliseEncoding<double, eckit::LocalConfiguration, eckit::LocalConfiguration, Options,

metkit::codes::CodesHandle>(

*(cacheEntry->impl_), Span<const double>{values}, mars, misc, opts_, language_);

Copilot · 2026-03-29T13:17:36Z

src/metkit/mars2grib/api/Mars2Grib.cc

+    try {
+        return CoreOperations::finaliseEncoding<float, eckit::LocalConfiguration, eckit::LocalConfiguration, Options,
+                                                metkit::codes::CodesHandle>(
+            *(cacheEntry->impl_), Span<const float>{values}, mars, misc, opts_, language_);
+    }
+    catch (...) {
+        throw eckit::Exception("Unable to finalise encoding", Here());
+    }


Same issue as the double overload: catching everything and throwing a fresh eckit::Exception drops the underlying nested exception context from CoreOperations::finaliseEncoding(...). Consider removing this try/catch or preserving the nested exception chain when wrapping.

Suggested change

try {

return CoreOperations::finaliseEncoding<float, eckit::LocalConfiguration, eckit::LocalConfiguration, Options,

metkit::codes::CodesHandle>(

*(cacheEntry->impl_), Span<const float>{values}, mars, misc, opts_, language_);

}

catch (...) {

throw eckit::Exception("Unable to finalise encoding", Here());

}

return CoreOperations::finaliseEncoding<float, eckit::LocalConfiguration, eckit::LocalConfiguration, Options,

metkit::codes::CodesHandle>(

*(cacheEntry->impl_), Span<const float>{values}, mars, misc, opts_, language_);

MircoValentiniECMWF added 3 commits March 27, 2026 22:11

Add copilot instructions for documentation checks

76a7efd

Initial version

04bf6dd

Add documentation

daf1aab

MircoValentiniECMWF requested review from Copilot, dsarmany and tweska March 29, 2026 00:49

Copilot started reviewing on behalf of MircoValentiniECMWF March 29, 2026 00:50 View session

Copilot AI reviewed Mar 29, 2026

View reviewed changes

Fix from copilot

969b937

MircoValentiniECMWF requested a review from Copilot March 29, 2026 12:45

Copilot started reviewing on behalf of MircoValentiniECMWF March 29, 2026 12:46 View session

Copilot AI reviewed Mar 29, 2026

View reviewed changes

MircoValentiniECMWF added 2 commits March 29, 2026 13:04

Fix bug on the stages

ea6ffc9

Run clang format

7a69124

MircoValentiniECMWF requested a review from Copilot March 29, 2026 13:13

Copilot started reviewing on behalf of MircoValentiniECMWF March 29, 2026 13:14 View session

Copilot AI reviewed Mar 29, 2026

View reviewed changes

		CacheEntryPtr prepare(const eckit::LocalConfiguration& mars, const eckit::LocalConfiguration& misc);

		// @todo eventually need to return another clone to commit modifications
		return samplePtr;

Conversation

MircoValentiniECMWF commented Mar 29, 2026

Description

Summary

What is included

SpecializedEncoder

CoreOperations

Mars2Grib API

Important note

Non-goals

Expected next step

Remarks

Contributor Declaration

Uh oh!

codecov-commenter commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

MircoValentiniECMWF Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

MircoValentiniECMWF Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

`SpecializedEncoder`

`CoreOperations`

`Mars2Grib` API

codecov-commenter commented Mar 29, 2026 •

edited

Loading