`cmdstan_version_compare()` conflates no version with old version
Nobody has claimed this yet.
- Dominant language
- R
- Stars
- 160
- Forks
- 69
- Avg merge
- 1d 19h
- Merged PRs (30d)
- 15
Description
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.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Locate cmdstan_version_compare() and cmdstan_default_path(), then inspect zzz.R and R/model.R:1391 before changing the sentinel behavior. Add regression coverage for missing, NA, empty, and older versions, and verify the package tests pass. Done means comparison results distinguish absent inputs from version ordering, path selection still works, and .onAttach() remains safe.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- r
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 65/100