Repository navigation
fixes for batched strategies - #57
Conversation
- use the hyperparameter prior_std instead of alpha - transition the sable backend and clients to use that feature, and clean up secification of unnecessary hyperparameters - fix and adapt benchmarks to the new vesion
- remove failing and commented out gpax tests - remove gpax tests that are backend independent (liar and indexed strategies) - add some sable tests that do pass
- otherwise, the loacl service is exposed to the local network, which is insecure by default
- if dim_y is not provided (None), infer it from the length of labels_y if possible.
- data normalization and transformation is done by the setter for dataset_x/y - it populates internal extracted/transformed private variables - remove cached_property decorators
- ensure that dataset_y with multiple rows is correctly handled - make the believer only believe the mean value, and lie about the standard deviation
- batch planning requires updating the model with fake (liar or believer) data in a loop. some surrogate models can gain speedup from using the pre-trianed model, and using less accurate training methods for these updates. Implement a general mechanism plus default implementation, and implement it fully for the sable backend.
| _yerr_train_raw = self.dataset_y[:, pos_yerr] | ||
| _yerr_train_raw = self._dataset_y[:, pos_yerr] |
There was a problem hiding this comment.
self.dataset_y property returns self._dataset_y. What is the point to explicitly use 'hidden' variable? Can self.dataset_y change in future?
There was a problem hiding this comment.
the setter for dataset_y calls this getter. I was afraid of a circular dependency, which may not be a problem. Is there a problem using the private variable here?
There was a problem hiding this comment.
It generally shouldn't matter whether or not we use the private variable in this instance, since we don't have any side effects in the getter functions. (I am strongly opposed to side effects in getter functions but we don't have any.)
| # this can not happen, how would we get a callable object through dial dataclass pydantic validation? | ||
| # data.strategy_args is type dict[str, float | int | bool | list[int | float]] | ||
| # elif callable(liar_setting): | ||
| # liar_value = liar_setting(data.dataset_y) |
There was a problem hiding this comment.
Not with current type options, but pydantic can support callables, e.g.
from collections.abc import Callable
from pydantic import BaseModel
class WorkerModel(BaseModel):
# Pydantic validates that 'callback' is a function/callable
callback: Callable[[int, str], bool]
# This works fine
def my_function(x: int, s: str) -> bool:
return True
model = WorkerModel(callback=my_function)
I prefer having an option for user defined callbacks
There was a problem hiding this comment.
Unfortunately, Callables will not work as a Dataclass type over INTERSECT. Under the hood INTERSECT needs to use the JSON conversion table (not the Python conversion table), and Callables are only valid under the Python conversion table
https://pydantic.dev/docs/validation/latest/concepts/conversion_table/
so we don't want Callable types in any of the dial_dataclass Pydantic BaseModels
There was a problem hiding this comment.
🟡 Changes recommended
Critical batching defects and moderate data/backend issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates batched strategies for transformed and heteroscedastic outputs, adds backend model-update support, improves validation, and restricts Docker services to localhost.
Changes:
- Reworks liar/believer pseudo-observation handling.
- Updates dataset transformations and dimension handling.
- Expands tests and adjusts Docker port bindings.
File summaries
| File | Review comments |
|---|---|
tests/unit/test_internals.py |
Add regression coverage for transformed, standardized, and heteroscedastic SABLE batching beyond shape assertions. Nit, 1 vote (line 1556). |
src/dial_service/utilities/strategies.py |
Initialize planning_model for non-indexed liar batching (critical, 3 votes, line 394); normalize scalar/list liar values into complete heteroscedastic rows (critical, 3 votes, line 321); add transformed and heteroscedastic liar/believer regression tests (nit, 1 vote, line 398); fix configureable typo (nit, 1 vote, line 359). |
src/dial_service/serverside_data.py |
Recompute or invalidate cached training data after bounds overrides (moderate, 3 votes, line 112); handle scalar negative Normal.scale values without indexing errors (moderate, 1 vote, line 98). |
src/dial_service/backends/sklearn_backend.py |
No review comments. |
src/dial_service/backends/sable_backend.py |
No review comments. |
src/dial_service/backends/gpax_backend.py |
No review comments. |
src/dial_service/backends/__init__.py |
Clarify or implement the abstract update_model fallback (moderate, 2 votes, line 38); fix learing typo (nit, 3 votes, line 42). |
src/dial_dataclass/dial_dataclass.py |
No review comments. |
docker-compose.yml |
No review comments. |
docker-compose-dev.yml |
No review comments. |
Review details
Suppressed comments (4)
src/dial_service/serverside_data.py:100
- For a constant negative
Normal.scale,_yerr_train_rawis a 0-D array. This branch indexes it withidxs[0], which raises an indexing error instead of the intended validationValueError; handle scalar errors separately or normalize the value to a 1-D array before indexing.
if np.any(_yerr_train_raw < 0):
idxs = np.where(_yerr_train_raw < 0)
msg = f'yerr values in statistics_y.scale must be non-negative, found {_yerr_train_raw[idxs[0]]} at {idxs[0]}.'
src/dial_service/utilities/strategies.py:359
- Typo:
configureableshould beconfigurable.
# could make this configureable, default to median right now
src/dial_service/utilities/strategies.py:405
- This change adds a regression-prone pseudo-observation path for transformed and heteroscedastic targets, but the batch tests still use scalar outputs; the heteroscedastic test only covers prediction. Add liar and believer cases with
Normal(loc='y', scale='yerr')and log/standardize preprocessing, including assertions that the original raw dataset is restored.
next_y = predictor(next_x).reshape(1, -1)
data.dataset_x = np.concatenate([data.dataset_x, next_x])
data.dataset_y = np.concatenate([data.dataset_y, next_y])
if data.strategy not in INDEXED_STRATEGIES:
# update_model updates the planning model, not modifying the original
planning_model = backend_module.update_model(planning_model, data)
tests/unit/test_internals.py:1559
- The SABLE branch only checks the returned shape, so it would pass even if the new inverse-transform logic or heteroscedastic pseudo-observations were incorrect. Since this PR targets
preprocess_log, standardization, andNormal(..., scale='yerr')with batching, add a regression case that exercises those settings and asserts the raw-data behavior rather than shape alone.
else:
# if we do not use sklearn based GP, there is no truth solution.
# Check only the shape of the return value
assert output.shape == (5, dim)
- Files reviewed: 10/10 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- "For a non-indexed strategy with batch_strategy='liar' (for example expected_improvement), planning_model is never initialized because the only assignment is in the believer branch, so the first iteration raises UnboundLocalError here. Initialize a private planning copy for the liar path as well" - "strategy_args accepts scalar and list liar_values, but a scalar becomes a 0-D array and a list is left without any assignment. With a heteroscedastic dataset_y shaped (n, 2) for Normal(loc='y', scale='yerr'), the scalar path produces a one-column pseudo-row that cannot be concatenated at line 398, while the list path raises on the unbound value."
Lance-Drane
left a comment
There was a problem hiding this comment.
LGTM, going to merge now
fixes #55