Skip to content

Framework improvements to generalize use of external model harness - #115

Merged
anagainaru merged 6 commits into
mainfrom
pr/framework-fixes
Aug 12, 2026
Merged

Framework improvements to generalize use of external model harness#115
anagainaru merged 6 commits into
mainfrom
pr/framework-fixes

Conversation

@S-Villar

@S-Villar S-Villar commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Six small fixes to the framework, found by trying to run a model harness that APEIRON had never been asked to run before. Together they help support some functionalities of a different type of model harness such as the MATEY foundation model, for which another PR is being prepared with an example in PR #116; these changes don't affect the behaviour for the bundled MNIST/CIFAR/ImageNet examples.

57 lines of source across nine files. Each commit is one fix and can be read on its own.

What was wrong

The framework quietly assumed certain patterns which might not be the case for all model harnesses:

A target is a bare Tensor. eval() weighted its metrics with y.size(0), which requires size to be a callable with Tensor's semantics:

torch.Tensor        y.size(0) -> 5                                       y.shape[0] -> 5
numpy.ndarray       y.size(0) -> TypeError: 'int' object is not callable y.shape[0] -> 5
structured target   y.size(0) -> AttributeError: no attribute 'size'     y.shape[0] -> 5

numpy's .size is an element count, not a method; a structured target has none at all. .shape is a plain attribute all three already expose, so reading it asks a custom batch type for one attribute rather than for Tensor's whole interface.

Eval metrics are self-describing. They are not, and the code already said so:

# TODO: need to find away to explicitly match the metrics to their name/label
cur_validation_metrics = self.modelHarness.eval()

eval() returns a bare list of floats, so the caller logged metrics[0] as test_curr_acc — genuinely accuracy for MNIST, electron-density error for a harness reporting seven metrics. The labels were never lost: eval_metrics is an ordered dict that eval() itself walks to build the list, so zipping its keys back on recovers every name. The pre-adaptation validation was also only printed, never recorded, so a finished run's CSV held post-adaptation numbers alone and no before/after comparison could be made from it.

Whoever calls get_logger() first sets the configuration. main() built the harness before configuring the logger, and get_logger() ignores its arguments once an instance exists — so a harness that logs during construction pinned the default backend and a null CSV path, and visualization.input was silently dropped for the whole run. No file, no error. Nothing in get_example() needs the logger, so configuring first is the entire fix.

Every profiled block does tensor work. With zero ATen events the FLOP profiler built a column on an empty DataFrame, which raises instead of recording zero. Reachable from stock config: update_mode = "none" does no work by design.

KSWIN can be replayed. It samples its reference window at random — the only one of the three detectors that samples anything. Three runs over identical data, before and after:

unseeded:  [165, 310]   [310]   [162, 311]
seeded:    [310]        [310]   [310]

evidently is importable everywhere. model_performance_detector.py imported it at module scope, and drift_detection/__init__.py imports that module — so import apeiron failed outright wherever evidently has no wheel, taking the three river-backed detectors down with it even though none of them needs it. It is now imported inside the single function that uses it. The public API is unchanged where evidently is installed.

New configuration

[drift_detection]
kswin_seed = 1337      # optional; unset keeps the historical unseeded behaviour

Documented in docs/configurations.md.

Summary

Any harness with a non-Tensor target, more than one eval metric, a no-op update mode, a need for reproducible detection, or a deployment without evidently hits at least one of these.

Testing

ruff check and ruff format --check clean. 207 passed.

One pre-existing failure, test_valiadation_tests.py::test_mnist_first_drift_losses_match_reference, reproduces identically on main — both branches produce [6.85099196434021, 5.589590549468994] against a reference of [7.514564037322998, 5.5480828285217285]. It needs the MNIST raw files present locally, so it silently skips without them; that is why it is easy to miss. This branch changes no numerics.

Two of the six ship without a dedicated test — the batch-size fix and the logger ordering — after the test simplification in review. Flagging it rather than letting it be found.

Note on an earlier revision

This branch was roughly six times larger before review. Removed since: [eval] max_val_batches (the per-window evaluation cap belongs in the harness's loader, which builds it and can bound it there), a drift-check counter and the run-summary lines it fed, a duplicate checkpointing implementation, and a test file that accounted for 88% of total suite wall time. continuous_monitor.py and test_continuous_monitor.py are now identical to main.

@S-Villar
S-Villar force-pushed the pr/framework-fixes branch from cd049e2 to b97ad65 Compare August 5, 2026 22:15
@S-Villar S-Villar changed the title Six framework fixes found bringing up an external model harness Framework fixes needed to run an external model harness Aug 5, 2026
@S-Villar S-Villar changed the title Framework fixes needed to run an external model harness Framework improvements to generalize use of external model harness Aug 5, 2026
@S-Villar
S-Villar marked this pull request as ready for review August 5, 2026 23:18
@S-Villar
S-Villar requested a review from anagainaru August 5, 2026 23:18

@anagainaru anagainaru left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe we don't need max_val_batches  and drift_event_count so I would remove this and make changes in the model harness/toml to include them.

I need to test the other model harnesses examples before we merge this

@S-Villar

S-Villar commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks — both done.

max_val_batches — removed. It capped how many batches the monitor evaluated per window, so a large validation split wouldn't cost hundreds of forward passes for one drift number. Agreed it belongs in the harness: get_stream_dataloader() already returns the loader being capped, so the harness can bound it there. continuous_monitor.py is now identical to main.

The counter — removed. One clarification: drift_event_count is already on main and still used for checkpoint naming, so it's not from this PR. The new one was drift_check_count, plus four run-summary log lines — those are what I removed. The count was already recoverable anyway, since each dispatch logs and writes cl/drift_event_id to the CSV.

Checkpointing — yes, reverted in 7a1df50.

Also: trimmed the comment on the lazy evidently import. It claimed evidently can't be installed everywhere, which contradicts pyproject.toml where it's a hard dependency. The import stays at the call site (otherwise import apeiron fails wherever evidently is missing, taking the statistical detectors with it), but the comment now says only that.

files lines
before 17 +401 / −18
now 12 +138 / −17

What's left: batch-size fix, logger ordering, KSWIN seed, lazy evidently import, per-name validation metrics, profiler guard. CI green.

I made the matching change in #116 so the stack stays consistent. No results changed — the cap was never binding.

@S-Villar
S-Villar force-pushed the pr/framework-fixes branch from 541ff25 to 9443ac7 Compare August 12, 2026 15:33
@anagainaru
anagainaru merged commit 2917101 into main Aug 12, 2026
3 checks passed
@anagainaru
anagainaru deleted the pr/framework-fixes branch August 12, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants