refactor: unify evaluate_* positional signatures#342
Open
mpvanderschelling wants to merge 1 commit into
Open
Conversation
Five `evaluate_*` functions formerly disagreed on the positional shape after `execute_fn`: - `evaluate_sequential(execute_fn, data, pass_id, **kwargs)` - `evaluate_multiprocessing(execute_fn, data, pass_id, nodes=..., **kwargs)` - `evaluate_cluster(execute_fn, data, pass_id, wait_for_creation=..., max_tries=..., **kwargs)` - `evaluate_mpi(execute_fn, comm, data, pass_id, ...)` -- `comm` injected at index 1 - `evaluate_cluster_array(execute_fn, data, job_number, pass_id, **kwargs)` -- `job_number` at index 2 The mismatch forced `DataGenerator.call` to know each mode's positional contract and ruled out cleanly accepting a generic evaluator plug-in (issue #309). Move `comm` and `job_number` into `**kwargs`. Each evaluator pops its required mode-specific arg from `kwargs` and raises a clear `TypeError` ("evaluate_mpi requires the MPI communicator via `comm=...`") if absent. The leading positional shape is now `(execute_fn, data, pass_id, **kwargs)` across all five functions, matching how `DataGenerator.call` already invoked them. `core.DataGenerator.call` already used keyword-only dispatch, so no caller update is needed. Existing tests already passed `comm=` and `job_number=` as keywords (see `tests/datageneration/test_datagenerator_modes.py`). Adds: - `test_cluster_array_missing_job_number_raises` -- omission surfaces as a clean TypeError instead of a deep KeyError. - `TestUnifiedEvaluateSignatures` -- introspection tests pinning the contract: all five evaluators must share the first three positional parameters and all must accept `**kwargs`. Fix #309 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #309. The five
evaluate_*functions formerly disagreed on the positional shape afterexecute_fn.evaluate_mpiinsertedcommat index 1;evaluate_cluster_arrayinsertedjob_numberat index 2. Both forcedDataGenerator.callto know each mode's positional contract.(execute_fn, data, pass_id, **kwargs)across all five functions.evaluate_mpinow pullscomm = kwargs.pop("comm")andevaluate_cluster_arraypullsjob_number = kwargs.pop("job_number")at the top of the function. Missing values surface as a cleanTypeError(evaluate_mpi requires the MPI communicator via \comm=...`) rather than a deepKeyError`.DataGenerator.callalready dispatched everything as keyword arguments, so no caller update is needed. Existing tests already passedcomm=andjob_number=as keywords.Test plan
uv run pytest tests/datageneration/ --no-cov— 81 passedtest_cluster_array_missing_job_number_raiseschecks the cleanTypeErrorTestUnifiedEvaluateSignaturesparametrises across all five evaluators and asserts (a) the first three positional parameters are(execute_fn, data, pass_id)and (b) each accepts**kwargs— a contract pin so the next change can't silently regressuv run pre-commit run ...— cleanNotes for reviewers
datagenerator.pytriplet (#247 → #234 → #309). Both prior branches are open againstdevelop; once one merges, the next will need a rebase. No conflict expected in_run_sampleor the helpers.Fix #309
🤖 Generated with Claude Code