changes per code review

Author: mitzimorris

cmdstanpy/utils.py

@@ -182,74 +182,74 @@ def cmdstan_path() -> str:
     return cmdstan
 
 
-def cmdstan_version_before(major: int, minor: int) -> bool:
+def cmdstan_version() -> Optional[Tuple[int, ...]]:
     """
-    Check that CmdStan version is less than Major.minor version.
-    Parses version string out of CmdStan makefile variable CMDSTAN_VERSION.
+    Parses version string out of CmdStan makefile variable CMDSTAN_VERSION,
+    returns Tuple(Major, minor).
 
-    If CmdStan installation is found but cannot parse version from makefile
-    logs warning and returns False.  Lenient behavoir required for CI tests,
-    per comment here:
+    If CmdStan installation is not found or cannot parse version from makefile
+    logs warning and returns None.  Lenient behavoir required for CI tests,
+    per comment:
     https://github.com/stan-dev/cmdstanpy/pull/321#issuecomment-733817554
-
-    :param major: Major version number
-    :param minor: Minor version number
-
-    :return: True if version at or above major.minor, else False.
     """
     try:
         makefile = os.path.join(cmdstan_path(), 'makefile')
     except ValueError:
-        get_logger().info(
-            'No CmdStan installation found, '
-            'cannot assert version is less than %d.%d.',
-            major,
-            minor,
-        )
-        return False
+        get_logger().info('No CmdStan installation found.')
+        return None
 
     if not os.path.exists(makefile):
         get_logger().info(
-            'CmdStan installation %s missing makefile, '
-            'cannot assert version is less than %d.%d.',
+            'CmdStan installation %s missing makefile, cannot get version.',
             cmdstan_path(),
-            major,
-            minor,
         )
-        return False
+        return None
 
     with open(makefile, 'r') as fd:
         contents = fd.read()
 
     start_idx = contents.find('CMDSTAN_VERSION := ')
     if start_idx < 0:
         get_logger().info(
-            'Cannot parse version from makefile: %s,'
-            'cannot assert version is less than %d.%d.',
+            'Cannot parse version from makefile: %s.',
             makefile,
-            major,
-            minor,
         )
-        return False
+        return None
 
     start_idx += len('CMDSTAN_VERSION := ')
     end_idx = contents.find('\n', start_idx)
 
     version = contents[start_idx:end_idx]
-    if version is None or len(version) < 3 or len(version.split('.')) < 2:
+    splits = version.split('.')
+    if len(splits) != 3:
         get_logger().info(
-            'Cannot parse version from makefile: %s,'
-            'cannot assert version is less than %d.%d.',
-            makefile,
-            major,
-            minor,
+            'Cannot parse version, expected "<major>.<minor>.<patch>", '
+            'found: "%s".',
+            version,
         )
-        return False
+        return None
+    return tuple(int(x) for x in splits[0:2])
 
-    splits = version.split('.')
-    cur_major = int(splits[0])
-    cur_minor = int(splits[1])
-    if cur_major < major or (cur_major == major and cur_minor < minor):
+
+def cmdstan_version_before(major: int, minor: int) -> bool:
+    """
+    Check that CmdStan version is less than Major.minor version.
+
+    :param major: Major version number
+    :param minor: Minor version number
+
+
+    :return: True if version at or above major.minor, else False.
+    """
+    cur_version = cmdstan_version()
+    if cur_version is None:
+        get_logger().info(
+            'Cannot determine whether version is before %d.%d.', major, minor
+        )
+        return False
+    if cur_version[0] < major or (
+        cur_version[0] == major and cur_version[1] < minor
+    ):
         return True
     return False
 

test/test_cmdstan_args.py

@@ -588,11 +588,12 @@ def test_args_sig_figs(self):
                 )
             expect = (
                 'Argument "sig_figs" invalid for CmdStan versions < 2.25, '
-                'using verson {} in directory {}'
+                'using version {} in directory {}'
             ).format(
                 os.path.basename(cmdstan_path()),
                 os.path.dirname(cmdstan_path()),
             )
+            print(log)
             log.check_present(('cmdstanpy', 'WARNING', expect))
         else:
             cmdstan_args = CmdStanArgs(

test/test_utils.py

@@ -13,6 +13,7 @@
 import string
 import tempfile
 import unittest
+from pathlib import Path
 
 import numpy as np
 import pandas as pd
@@ -28,6 +29,7 @@
     TemporaryCopiedFile,
     check_sampler_csv,
     cmdstan_path,
+    cmdstan_version,
     cmdstan_version_before,
     do_command,
     flatten_chains,
@@ -205,27 +207,43 @@ def test_munge_cmdstan_versions(self):
     def test_cmdstan_version_before(self):
         cmdstan_path()  # sets os.environ['CMDSTAN']
         self.assertTrue(cmdstan_version_before(99, 99))
+        self.assertFalse(cmdstan_version_before(1, 1))
 
+    def test_cmdstan_version(self):
         with tempfile.TemporaryDirectory(
             prefix="cmdstan_tests", dir=_TMPDIR
         ) as tmpdir:
             tdir = os.path.join(tmpdir, 'tmpdir_xxx')
             os.makedirs(tdir)
-            os.makedirs(os.path.join(tdir, 'cmdstan-2.22.0'))
-            os.environ['CMDSTAN'] = os.path.join(tdir, 'cmdstan-2.22.0')
+            fake_path = os.path.join(tdir, 'cmdstan-2.22.0')
+            os.makedirs(os.path.join(fake_path))
+            fake_bin = os.path.join(fake_path, 'bin')
+            os.makedirs(fake_bin)
+            Path(os.path.join(fake_bin, 'stanc' + EXTENSION)).touch()
+            os.environ['CMDSTAN'] = fake_path
+            self.assertTrue(fake_path == cmdstan_path())
+            expect = (
+                'CmdStan installation {} missing makefile, '
+                'cannot get version.'.format(fake_path)
+            )
             with LogCapture() as log:
                 logging.getLogger()
-                self.assertFalse(cmdstan_version_before(99, 99))
-                del os.environ['CMDSTAN']
-                cmdstan_path()
-            log.check_present(
-                (
-                    'cmdstanpy',
-                    'INFO',
-                    'No CmdStan installation found, '
-                    'cannot assert version is less than 99.99.',
-                )
+                cmdstan_version()
+            log.check_present(('cmdstanpy', 'INFO', expect))
+            fake_makefile = os.path.join(fake_path, 'makefile')
+            with open(fake_makefile, 'w') as fd:
+                fd.write('...  CMDSTAN_VERSION := dont_need_no_mmp\n\n')
+            expect = (
+                'Cannot parse version, expected "<major>.<minor>.<patch>", '
+                'found: "dont_need_no_mmp".'
             )
+            with LogCapture() as log:
+                logging.getLogger()
+                cmdstan_version()
+            log.check_present(('cmdstanpy', 'INFO', expect))
+        # cleanup
+        del os.environ['CMDSTAN']
+        cmdstan_path()
 
 
 class DataFilesTest(unittest.TestCase):