Address PR comments for python parameters impl

Author: joshuawootonn

py/src/braintrust/cli/eval.py

@@ -133,7 +133,7 @@ async def run_evaluator_task(evaluator, position, opts: EvaluatorOpts):
             dataset = evaluator.data
 
         parameters = None
-        if RemoteEvalParameters.is_parameters(evaluator.parameters) and evaluator.parameters.id is not None:
+        if isinstance(evaluator.parameters, RemoteEvalParameters) and evaluator.parameters.id is not None:
             parameters = {"id": evaluator.parameters.id}
             if evaluator.parameters.version is not None:
                 parameters["version"] = evaluator.parameters.version

py/src/braintrust/devserver/eval_hooks.py

@@ -10,14 +10,16 @@
 from collections.abc import Callable
 from typing import Any
 
+from ..parameters import ValidatedParameters
+
 
 class EvalHooks:
     """Hooks provided to eval tasks for progress reporting."""
 
     def __init__(
         self,
         report_progress: Callable[[dict[str, Any]], None] | None = None,
-        parameters: dict[str, Any] | None = None,
+        parameters: ValidatedParameters | None = None,
     ):
         self._report_progress = report_progress
         self.parameters = parameters or {}

py/src/braintrust/devserver/server.py

@@ -25,10 +25,22 @@
         )
     )
 
-from ..framework import EvalAsync, EvalScorer, Evaluator, ExperimentSummary, SSEProgressEvent
+from ..framework import (
+    EvalAsync,
+    EvalHooks,
+    EvalScorer,
+    Evaluator,
+    ExperimentSummary,
+    SSEProgressEvent,
+)
 from ..generated_types import FunctionId
 from ..logger import BraintrustState, bt_iscoroutinefunction
-from ..parameters import RemoteEvalParameters, serialize_remote_eval_parameters_container, validate_parameters
+from ..parameters import (
+    RemoteEvalParameters,
+    ValidatedParameters,
+    serialize_remote_eval_parameters_container,
+    validate_parameters,
+)
 from ..span_identifier_v4 import parse_parent
 from .auth import AuthorizationMiddleware
 from .cache import cached_login
@@ -42,39 +54,16 @@
 
 
 class _ParameterOverrideHooks:
-    def __init__(self, hooks: Any, parameters: dict[str, Any]):
+    def __init__(self, hooks: EvalHooks[Any], parameters: ValidatedParameters):
         self._hooks = hooks
         self._parameters = parameters
 
     @property
-    def metadata(self):
-        return self._hooks.metadata
-
-    @property
-    def expected(self):
-        return self._hooks.expected
-
-    @property
-    def span(self):
-        return self._hooks.span
-
-    @property
-    def trial_index(self):
-        return self._hooks.trial_index
-
-    @property
-    def tags(self):
-        return self._hooks.tags
-
-    @property
-    def parameters(self):
+    def parameters(self) -> ValidatedParameters:
         return self._parameters
 
-    def report_progress(self, progress):
-        return self._hooks.report_progress(progress)
-
-    def meta(self, **info: Any):
-        return self._hooks.meta(**info)
+    def __getattr__(self, name: str):
+        return getattr(self._hooks, name)
 
 
 class CheckAuthorizedMiddleware(BaseHTTPMiddleware):
@@ -192,7 +181,7 @@ async def run_eval(request: Request) -> JSONResponse | StreamingResponse:
     # Set up SSE headers for streaming
     sse_queue = SSEQueue()
 
-    async def task(input, hooks):
+    async def task(input: Any, hooks: EvalHooks[Any]):
         task_hooks = hooks if validated_parameters is None else _ParameterOverrideHooks(hooks, validated_parameters)
         if bt_iscoroutinefunction(evaluator.task):
             result = await evaluator.task(input, task_hooks)
@@ -228,7 +217,7 @@ def stream_fn(event: SSEProgressEvent):
     eval_kwargs = {
         k: v for (k, v) in evaluator.__dict__.items() if k not in ["eval_name", "project_name", "parameter_values"]
     }
-    if validated_parameters is not None and not RemoteEvalParameters.is_parameters(evaluator.parameters):
+    if validated_parameters is not None and not isinstance(evaluator.parameters, RemoteEvalParameters):
         eval_kwargs["parameters"] = validated_parameters
 
     try:
@@ -329,7 +318,10 @@ def create_app(evaluators: list[Evaluator[Any, Any]], org_name: str | None = Non
 
 
 def run_dev_server(
-    evaluators: list[Evaluator[Any, Any]], host: str = "localhost", port: int = 8300, org_name: str | None = None
+    evaluators: list[Evaluator[Any, Any]],
+    host: str = "localhost",
+    port: int = 8300,
+    org_name: str | None = None,
 ):
     """Start the dev server.
 

py/src/braintrust/framework.py

@@ -42,7 +42,13 @@
     stringify_exception,
 )
 from .logger import init as _init_experiment
-from .parameters import EvalParameters, RemoteEvalParameters, is_eval_parameter_schema, validate_parameters
+from .parameters import (
+    EvalParameters,
+    RemoteEvalParameters,
+    ValidatedParameters,
+    is_eval_parameter_schema,
+    validate_parameters,
+)
 from .resource_manager import ResourceManager
 from .score import Score, is_score, is_scorer
 from .serializable_data_class import SerializableDataClass
@@ -215,7 +221,7 @@ def meta(self, **info: Any) -> None:
 
     @property
     @abc.abstractmethod
-    def parameters(self) -> dict[str, Any] | None:
+    def parameters(self) -> ValidatedParameters | None:
         """
         The parameters for the current evaluation. These are the validated parameter values
         that were passed to the evaluator.
@@ -744,7 +750,7 @@ async def make_empty_summary():
             dataset = evaluator.data
 
         experiment_parameters = None
-        if RemoteEvalParameters.is_parameters(evaluator.parameters) and evaluator.parameters.id is not None:
+        if isinstance(evaluator.parameters, RemoteEvalParameters) and evaluator.parameters.id is not None:
             experiment_parameters = {"id": evaluator.parameters.id}
             if evaluator.parameters.version is not None:
                 experiment_parameters["version"] = evaluator.parameters.version
@@ -1162,7 +1168,7 @@ def __init__(
         trial_index: int = 0,
         tags: Sequence[str] | None = None,
         report_progress: Callable[[TaskProgressEvent], None] = None,
-        parameters: dict[str, Any] | None = None,
+        parameters: ValidatedParameters | None = None,
     ):
         if metadata is not None:
             self.update({"metadata": metadata})
@@ -1220,7 +1226,7 @@ def report_progress(self, event: TaskProgressEvent):
             return self._report_progress(event)
 
     @property
-    def parameters(self) -> dict[str, Any] | None:
+    def parameters(self) -> ValidatedParameters | None:
         return self._parameters
 
 
@@ -1403,7 +1409,7 @@ def get_other_fields(s):
 
     if evaluator.parameter_values is not None:
         resolved_evaluator_parameters = evaluator.parameter_values
-    elif RemoteEvalParameters.is_parameters(evaluator.parameters):
+    elif isinstance(evaluator.parameters, RemoteEvalParameters):
         resolved_evaluator_parameters = validate_parameters({}, evaluator.parameters)
     elif is_eval_parameter_schema(evaluator.parameters):
         resolved_evaluator_parameters = validate_parameters({}, evaluator.parameters)

py/src/braintrust/logger.py

@@ -2044,7 +2044,7 @@ def _get_parameters_ref(
 ) -> ParametersRef | None:
     if parameters is None:
         return None
-    if RemoteEvalParameters.is_parameters(parameters):
+    if isinstance(parameters, RemoteEvalParameters):
         if parameters.id is None:
             return None
         ref: ParametersRef = {"id": parameters.id}
@@ -2085,43 +2085,39 @@ def load_parameters(
     :returns: A `RemoteEvalParameters` object.
     """
     if version is not None and environment is not None:
-        raise ValueError(
-            "Cannot specify both 'version' and 'environment' parameters. Please use only one (remove the other)."
-        )
+        raise ValueError("Cannot specify both 'version' and 'environment' parameters.")
 
-    if id:
-        pass
-    elif not project and not project_id:
+    if id is None and not project and not project_id:
         raise ValueError("Must specify at least one of project or project_id")
-    elif not slug:
+    if id is None and not slug:
         raise ValueError("Must specify slug")
 
+    should_fall_back_to_cache = version is None and environment is None
+    query_args = _populate_args(
+        {
+            "version": version,
+            "environment": environment,
+        }
+    )
+
     try:
         login(org_name=org_name, api_key=api_key, app_url=app_url)
         if id:
-            parameters_args = {}
-            if version is not None:
-                parameters_args["version"] = version
-            if environment is not None:
-                parameters_args["environment"] = environment
-            response = _state.api_conn().get_json(f"/v1/function/{id}", parameters_args)
+            response = _state.api_conn().get_json(f"/v1/function/{id}", query_args)
             if response is not None:
                 response = {"objects": [response]}
         else:
-            args = _populate_args(
-                {
-                    "project_name": project,
-                    "project_id": project_id,
-                    "slug": slug,
-                    "version": version,
-                    "environment": environment,
-                    "function_type": "parameters",
-                }
-            )
+            args = {
+                "project_name": project,
+                "project_id": project_id,
+                "slug": slug,
+                "function_type": "parameters",
+                **query_args,
+            }
             response = _state.api_conn().get_json("/v1/function", args)
     except Exception as server_error:
-        if environment is not None or version is not None:
-            raise ValueError("Parameters not found with specified parameters") from server_error
+        if not should_fall_back_to_cache:
+            raise
 
         eprint(f"Failed to load parameters, attempting to fall back to cache: {server_error}")
         try:
@@ -2139,7 +2135,7 @@ def load_parameters(
                     f"Parameters with id {id} not found (not found on server or in local cache): {cache_error}"
                 ) from server_error
             raise ValueError(
-                f"Parameters {slug} (version {version or 'latest'}) not found in {project or project_id} (not found on server or in local cache): {cache_error}"
+                f"Parameters {slug} not found in {project or project_id} (not found on server or in local cache): {cache_error}"
             ) from server_error
 
     if response is None or "objects" not in response or len(response["objects"]) == 0: