Skip to content

adding cmdstanr as backend in shared params #57

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

micahwiesner67
Copy link
Collaborator

@micahwiesner67 micahwiesner67 commented Apr 25, 2025

We need to add the backend ("cmdstanr" or "rstan") in for the v2 release of epinow2pipeline

@micahwiesner67
Copy link
Collaborator Author

This is a pre-requisite for running the cfa-epinow2 v2 code. Feel free to approve whenever you have the opportunity and we can discuss when to pull this in

Copy link
Contributor

@natemcintosh natemcintosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the epinow2 pipeline will read the new backend field, and pick the right one at runtime?

@micahwiesner67
Copy link
Collaborator Author

Yes, it will read it from this config. Technically we could run from either 'cmdstanr' or 'rstan' after the v2 release goes in, but from talking to Zach I believe we just want to support 'cmdstanr' going forward

@athowes
Copy link
Collaborator

athowes commented May 13, 2025

Is this a breaking change if we were to merge it now? (Before the V2 release of the pipeline.)

@micahwiesner67
Copy link
Collaborator Author

@athowes It shouldn't be. This should be able to be added before v3 release of pipeline, we would just need to change the backend to 'rstan' to be accurate (this parameter wouldn't be being used, but for documentation sake should be accurate)

@athowes
Copy link
Collaborator

athowes commented May 15, 2025

Yes my suggestion is that if this can go in in advance we might as well do that rather than have it sitting as an open PR!

Copy link
Contributor

@zsusswein zsusswein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a totally fine small change.

But! I think we should bump the config version because this is a breaking change for going forward. And perhaps we should think about allowing the pipeline to parse thing config_version field but that's probably overkill.

getting update to make package
@micahwiesner67
Copy link
Collaborator Author

I created this branch just to demonstrate a basic way to test the 'rstan' backend. Can you check it out locally in VAP and just run 'make test-batch'. The following branch isn't meant to be pulled in, btw.

Canyo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants