Implement initial positional-arg deprecation decorator

.pre-commit-config.yaml

@@ -16,7 +16,7 @@ repos:
     hooks:
       - id: black
   - repo: https://github.com/pycqa/flake8
-    rev: 5.0.4
+    rev: 7.3.0
     hooks:
       - id: flake8
   - repo: https://github.com/pre-commit/mirrors-mypy

cmdstanpy/model.py

@@ -46,6 +46,7 @@
     do_command,
     get_logger,
 )
+from cmdstanpy.utils.deprecation import deprecate_positional_args
 from cmdstanpy.utils.filesystem import (
     temp_inits,
     temp_metrics,
@@ -304,9 +305,11 @@ def code(self) -> Optional[str]:
             )
         return code
 
+    @deprecate_positional_args(last_deprecated="jacobian")
     def optimize(
         self,
         data: Union[Mapping[str, Any], str, os.PathLike, None] = None,
+        *,
         seed: Optional[int] = None,
         inits: Union[Mapping[str, Any], float, str, os.PathLike, None] = None,
         output_dir: OptionalPath = None,
@@ -328,7 +331,6 @@ def optimize(
         time_fmt: str = "%Y%m%d%H%M%S",
         timeout: Optional[float] = None,
         jacobian: bool = False,
-        # would be nice to move this further up, but that's a breaking change
     ) -> CmdStanMLE:
         """
         Run the specified CmdStan optimize algorithm to produce a
@@ -496,9 +498,11 @@ def optimize(
         return mle
 
     # pylint: disable=too-many-arguments
+    @deprecate_positional_args(last_deprecated="timeout")
     def sample(
         self,
         data: Union[Mapping[str, Any], str, os.PathLike, None] = None,
+        *,
         chains: Optional[int] = None,
         parallel_chains: Optional[int] = None,
         threads_per_chain: Optional[int] = None,
@@ -536,7 +540,6 @@ def sample(
         refresh: Optional[int] = None,
         time_fmt: str = "%Y%m%d%H%M%S",
         timeout: Optional[float] = None,
-        *,
         force_one_process_per_chain: Optional[bool] = None,
         inv_metric: Union[
             str,
@@ -1034,18 +1037,19 @@ def sample(
             mcmc = CmdStanMCMC(runset)
         return mcmc
 
+    @deprecate_positional_args(last_deprecated="timeout")
     def generate_quantities(
         self,
         data: Union[Mapping[str, Any], str, os.PathLike, None] = None,
         previous_fit: Union[Fit, list[str], None] = None,
+        *,
         seed: Optional[int] = None,
         gq_output_dir: OptionalPath = None,
         sig_figs: Optional[int] = None,
         show_console: bool = False,
         refresh: Optional[int] = None,
         time_fmt: str = "%Y%m%d%H%M%S",
         timeout: Optional[float] = None,
-        *,
         mcmc_sample: Union[CmdStanMCMC, list[str], None] = None,
     ) -> CmdStanGQ[Fit]:
         """
@@ -1227,9 +1231,11 @@ def generate_quantities(
             quantities = CmdStanGQ(runset=runset, previous_fit=fit_object)
         return quantities
 
+    @deprecate_positional_args(last_deprecated="timeout")
     def variational(
         self,
         data: Union[Mapping[str, Any], str, os.PathLike, None] = None,
+        *,
         seed: Optional[int] = None,
         inits: Optional[float] = None,
         output_dir: OptionalPath = None,
@@ -1251,7 +1257,6 @@ def variational(
         refresh: Optional[int] = None,
         time_fmt: str = "%Y%m%d%H%M%S",
         timeout: Optional[float] = None,
-        *,
         output_samples: Optional[int] = None,
         save_latent_dynamics: Optional[bool] = None,
     ) -> CmdStanVB:

cmdstanpy/utils/cmdstan.py

@@ -14,6 +14,7 @@
 from cmdstanpy import _DOT_CMDSTAN
 
 from .. import progress as progbar
+from .deprecation import deprecate_positional_args
 from .logging import get_logger
 
 EXTENSION = '.exe' if platform.system() == 'Windows' else ''
@@ -433,15 +434,16 @@ def cxx_toolchain_path(
     return compiler_path, tool_path
 
 
+@deprecate_positional_args(last_deprecated="cores")
 def install_cmdstan(
     version: Optional[str] = None,
+    *,
     dir: Optional[str] = None,
     overwrite: bool = False,
     compiler: bool = False,
     progress: bool = False,
     verbose: bool = False,
     cores: int = 1,
-    *,
     interactive: bool = False,
 ) -> bool:
     """

cmdstanpy/utils/deprecation.py

@@ -0,0 +1,58 @@
+"""
+Temporary helpers for deprecations in the 1.x series.
+"""
+
+import functools
+import inspect
+from typing import Any, Callable, TypeVar
+
+from .logging import get_logger
+
+F = TypeVar('F', bound=Callable[..., Any])
+
+
+def deprecate_positional_args(last_deprecated: str) -> Callable[[F], F]:
+    """
+    Allow keyword-only arguments up to and including ``last_deprecated``
+    to still be passed by position, logging a deprecation warning.
+
+    Positional use of these arguments becomes an error in CmdStanPy 2.0.
+    """
+
+    def decorator(func: F) -> F:
+        params = inspect.signature(func).parameters.values()
+        n_positional = sum(
+            p.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD for p in params
+        )
+        kw_only = [
+            p.name for p in params if p.kind == inspect.Parameter.KEYWORD_ONLY
+        ]
+        deprecated = kw_only[: kw_only.index(last_deprecated) + 1]
+
+        @functools.wraps(func)
+        def wrapper(*args: Any, **kwargs: Any) -> Any:
+            extra = args[n_positional:]
+            if not extra or len(extra) > len(deprecated):
+                # either nothing to do, or let Python raise its usual error
+                return func(*args, **kwargs)
+            names = deprecated[: len(extra)]
+            for name in names:
+                if name in kwargs:
+                    raise TypeError(
+                        f"{func.__qualname__}() got multiple values for "
+                        f"argument '{name}'"
+                    )
+            quoted = ", ".join("`" + name + "`" for name in names)
+            get_logger().warning(
+                "Passing %s to %s() by position is deprecated and will raise "
+                "an error in CmdStanPy 2.0. Please pass them as keyword "
+                "arguments instead.",
+                quoted,
+                func.__qualname__,
+            )
+            kwargs.update(zip(names, extra))
+            return func(*args[:n_positional], **kwargs)
+
+        return wrapper  # type: ignore[return-value]
+
+    return decorator

test/test_deprecations.py

@@ -0,0 +1,140 @@
+"""Tests for deprecated positional arguments to public functions"""
+
+# positional calls below are deliberate
+# pylint: disable=too-many-function-args
+
+import inspect
+import logging
+import os
+import re
+import sys
+from test import check_present
+from typing import Any, Callable
+from unittest import mock
+
+import pytest
+
+from cmdstanpy.model import CmdStanModel
+from cmdstanpy.utils import install_cmdstan
+from cmdstanpy.utils.deprecation import deprecate_positional_args
+
+HERE = os.path.dirname(os.path.abspath(__file__))
+DATAFILES_PATH = os.path.join(HERE, 'data')
+
+
+class Dummy:
+    @deprecate_positional_args(last_deprecated="c")
+    def method(
+        self, a: Any = None, *, b: Any = None, c: Any = None, d: Any = None
+    ) -> tuple[Any, Any, Any, Any]:
+        return a, b, c, d
+
+
+def test_keywords_do_not_warn(caplog: pytest.LogCaptureFixture) -> None:
+    with caplog.at_level(logging.WARNING):
+        assert Dummy().method(1, b=2, c=3, d=4) == (1, 2, 3, 4)
+        assert Dummy().method() == (None, None, None, None)
+    assert not caplog.records
+
+
+def test_positional_maps_to_keywords(caplog: pytest.LogCaptureFixture) -> None:
+    with caplog.at_level(logging.WARNING):
+        assert Dummy().method(1, 2, 3, d=4) == (1, 2, 3, 4)
+    check_present(
+        caplog,
+        (
+            'cmdstanpy',
+            'WARNING',
+            re.compile(r"Passing `b`, `c` to Dummy\.method\(\) by position"),
+        ),
+    )
+
+
+def test_positional_and_keyword_collision() -> None:
+    with pytest.raises(TypeError, match="multiple values for argument 'b'"):
+        Dummy().method(1, 2, b=3)
+
+
+def test_too_many_positional() -> None:
+    # `d` was never positional, so it must not be filled by position
+    with pytest.raises(TypeError):
+        Dummy().method(1, 2, 3, 4)
+
+
+def test_signature_is_keyword_only() -> None:
+    params = inspect.signature(Dummy.method).parameters
+    assert params['a'].kind == inspect.Parameter.POSITIONAL_OR_KEYWORD
+    assert params['b'].kind == inspect.Parameter.KEYWORD_ONLY
+
+
+@pytest.mark.parametrize(
+    'func, positional, last_deprecated',
+    [
+        (CmdStanModel.sample, ['self', 'data'], 'timeout'),
+        (CmdStanModel.optimize, ['self', 'data'], 'jacobian'),
+        (CmdStanModel.variational, ['self', 'data'], 'timeout'),
+        (
+            CmdStanModel.generate_quantities,
+            ['self', 'data', 'previous_fit'],
+            'timeout',
+        ),
+        (install_cmdstan, ['version'], 'cores'),
+    ],
+)
+def test_public_signatures(
+    func: Callable[..., Any], positional: list[str], last_deprecated: str
+) -> None:
+    params = inspect.signature(func).parameters
+    kinds = {name: p.kind for name, p in params.items()}
+    assert [
+        n
+        for n, k in kinds.items()
+        if k == inspect.Parameter.POSITIONAL_OR_KEYWORD
+    ] == positional
+    # guards against a rename silently shifting the positional mapping
+    assert kinds[last_deprecated] == inspect.Parameter.KEYWORD_ONLY
+
+
+def test_optimize_positional_seed(caplog: pytest.LogCaptureFixture) -> None:
+    stan = os.path.join(DATAFILES_PATH, 'bernoulli.stan')
+    jdata = os.path.join(DATAFILES_PATH, 'bernoulli.data.json')
+    model = CmdStanModel(stan_file=stan)
+    with caplog.at_level(logging.WARNING):
+        mle_pos = model.optimize(jdata, 1234)
+    check_present(
+        caplog,
+        (
+            'cmdstanpy',
+            'WARNING',
+            re.compile(r"Passing `seed` to CmdStanModel\.optimize\(\)"),
+        ),
+    )
+    mle_kw = model.optimize(jdata, seed=1234)
+    assert mle_pos.metadata.cmdstan_config['seed'] == 1234
+    assert mle_pos.optimized_params_dict == mle_kw.optimized_params_dict
+
+
+@pytest.mark.skipif(
+    sys.version_info < (3, 11),
+    reason="mock.patch resolves `cmdstanpy.install_cmdstan` to the "
+    "re-exported function, not the module, before Python 3.11",
+)
+def test_install_cmdstan_positional(caplog: pytest.LogCaptureFixture) -> None:
+    # stop right after argument handling; nothing is downloaded
+    with mock.patch(
+        'cmdstanpy.install_cmdstan.run_install', side_effect=RuntimeError
+    ) as run_install:
+        with caplog.at_level(logging.WARNING):
+            install_cmdstan("2.36.0", "some_dir", True)
+    check_present(
+        caplog,
+        (
+            'cmdstanpy',
+            'WARNING',
+            re.compile(r"Passing `dir`, `overwrite` to install_cmdstan\(\)"),
+        ),
+    )
+    settings = run_install.call_args.args[0]
+    assert settings.version == "2.36.0"
+    assert settings.dir == "some_dir"
+    assert settings.overwrite