Skip to content

cmdstan_version_compare() conflates no version with old version #1260

Description

@jgabry

cmdstan_version_compare() returns -1 when the first argument is missing, NA, or an empty string because path discovery uses "" when it doesn't find native or WSL installs. That works well for cmdstan_default_path(). But -1 is also used for indicating that a version is older and it's not possible after the fact to tell the two uses of -1 apart.

I think cmdstan_version_compare() should just compare versions. cmdstan_default_path() can then just return early when it needs to, e.g.:

if (!nzchar(latest_cmdstan)) return(file.path(wsl_installs_path, latest_wsl_cmdstan))
if (!nzchar(latest_wsl_cmdstan)) return(file.path(installs_path, latest_cmdstan))
if (cmdstan_version_compare(latest_wsl_cmdstan, latest_cmdstan) >= 0) ...

We also need to be careful about zzz.R since it runs during .onAttach(). Right now the comparison isn't wrapped in try() the way the version lookups are, so it should probably be moved inside try() or handled some other way.

When running this by claude it pointed out that there's a tempting fix that should be avoided. In it's own words:

Making model_compile_info() return NULL instead of ".." looks cheaper and is worse. NULL hits the sentinel, so $sample()'s gate at R/model.R:1391 silently takes the "older than 2.36.0" branch instead of erroring. That trades a loud failure in the wrong place for a quiet wrong answer.

Refusing bad versions at the source belongs with the executable adoption work, which should decline to construct a model from a binary that cannot report a version (related: #1246). This issue is about the comparison, which should not answer a question it was not asked.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions