diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index c8a219e1..920a990c 100644 --- a/.pre-commit-config.yaml +++ b/.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 diff --git a/cmdstanpy/model.py b/cmdstanpy/model.py index 7e509981..fdd21798 100644 --- a/cmdstanpy/model.py +++ b/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,10 +1037,12 @@ 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, @@ -1045,7 +1050,6 @@ def generate_quantities( 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: diff --git a/cmdstanpy/utils/cmdstan.py b/cmdstanpy/utils/cmdstan.py index 60b6551e..a5848024 100644 --- a/cmdstanpy/utils/cmdstan.py +++ b/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: """ diff --git a/cmdstanpy/utils/deprecation.py b/cmdstanpy/utils/deprecation.py new file mode 100644 index 00000000..55e034b2 --- /dev/null +++ b/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 diff --git a/test/test_deprecations.py b/test/test_deprecations.py new file mode 100644 index 00000000..3a7b36e4 --- /dev/null +++ b/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