Skip to content

Cache only the encoding plan to avoid inconsistencies #193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
MircoValentiniECMWF wants to merge 1 commit into
base: develop
Choose a base branch
from
+3 −3
Draft

Cache only the encoding plan to avoid inconsistencies #193

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/metkit/mars2grib/CoreOperations.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ struct CoreOperations {
/// prepares the structure required for a future internal cache design.
///
CacheEntry(Layout&& layout, const MarsDict_t& inputMars, const ParDict_t& inputMisc, const OptDict_t& options) :
encoder_{std::move(layout)}, preparedSample_{encoder_.prepare(inputMars, inputMisc, options)} {};
encoder_{std::move(layout)} {};
Comment on lines 359 to +362
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

CacheEntry no longer prepares/caches an immutable sample, but the constructor’s Doxygen comment still says it “prepares an immutable reusable sample from the active metadata” and the type-level docs describe a two-phase (prepare/finalise) pipeline. Please update the documentation (and potentially the constructor signature/call sites) to reflect that this cache entry now only caches the resolved layout/encoding plan.

Copilot uses AI. Check for mistakes.

CacheEntry(const CacheEntry&) = delete;
CacheEntry& operator=(const CacheEntry&) = delete;
Expand All @@ -383,7 +383,7 @@ struct CoreOperations {
/// during `finaliseEncoding()`, which derives a fresh output handle
/// from it.
///
const std::unique_ptr<const OutDict_t> preparedSample_;
// const std::unique_ptr<const OutDict_t> preparedSample_;
};

///
Expand Down Expand Up @@ -551,7 +551,7 @@ struct CoreOperations {
normalize_if_enabled(inputMars, inputMisc, options, language, scratchMars, scratchMisc);

auto gribHeader =
cacheEntry.encoder_.finaliseEncoding(*(cacheEntry.preparedSample_), activeMars, activeMisc, options);
cacheEntry.encoder_.encode( activeMars, activeMisc, options);

return encodeValues(values, activeMisc, options, std::move(gribHeader));
Comment on lines 553 to 556
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

There are no tests covering the staged cache path (prepare() + finaliseEncoding()), and this change materially alters that behavior. Please add regression coverage (e.g., asserting that finaliseEncoding(prepare(mars,misc), values, mars, misc) produces the same header/handle keys as encode(values, mars, misc) for representative inputs, and that calling finaliseEncoding() multiple times with the same cache is stable).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@dsarmany dsarmany Apr 9, 2026

Choose a reason for hiding this comment

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

This cache is still quite slow for AIFS.

}
Expand Down
Loading