Skip to content
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

Include BridgeStan using git subtree #59

Merged
merged 5 commits into from Oct 23, 2022
Merged

Conversation

sethaxen
Copy link
Contributor

This PR relates StanJulia/Stan.jl#121 by including bridgestan in this repo using git subtree. Note that I wasn't able to completely silence the warning emitted by BridgeStan about its path not being set. This is because we can set the path at include time and at load time of StanSample, but the check is performed in the __init__ function of BridgeStan, which seems to be called before StanSample.__init__, so the warning is raised but can be safely ignored. It seems like there should be some way around this. Perhaps BridgeStan should be lazily performing this check instead of at load time.

cc @goedman

@goedman goedman merged commit c23486e into StanJulia:master Oct 23, 2022
@sethaxen sethaxen deleted the bridgestan branch October 23, 2022 19:55
@goedman
Copy link
Collaborator

goedman commented Oct 23, 2022

Thanks for this! It works.

The warning goes away if BRIDGESTAN is defined, even when not pointing to a correct location. I expect it should be something like ./StanSample/deps/data/bridgestan?

This warning does not seem to be very useful used this way.

@goedman
Copy link
Collaborator

goedman commented Oct 24, 2022

Hi @sethaxen

I noticed a path (bridge-path) set in SampleModel was no longer correct for the new location of BridgeStan. I'm fixing that in StanSample.jl v6.11.1.

@goedman
Copy link
Collaborator

goedman commented Oct 24, 2022

It looks like v6.11.1 works on my system. Problems started when I removed the old bridgestan download which caused the above bridge-path issue.

I'll contact Edward to see what we can do about the initial warning. Although we can ignore the warning, I don't think that should happen.

On CI I see a bigger problem. It can't build the BridgeStan library. I don't think I understand how including the BridgeStan subtree is supposed to work and what triggers the creation of deps/data/bridgestan. Should I add debs/data to .gitignore?

@sethaxen
Copy link
Contributor Author

On CI I see a bigger problem. It can't build the BridgeStan library. I don't think I understand how including the BridgeStan subtree is supposed to work and what triggers the creation of deps/data/bridgestan. Should I add debs/data to .gitignore?

No, that shouldn't be necessary. Git subtree works by adding the bridgestan repo as part of this repo, but with a nice mechanism for keeping it up-to-date (as opposed to submodule, which would just add a link to a commit in the other repo; this is not allowed by Julia's package manager). Whenever StanSample is installed, BridgeStan is automatically there.

I can look into what's going wrong in the CI. Can you point me to a failing job?

@goedman
Copy link
Collaborator

goedman commented Oct 24, 2022

This one here if you look at the output of running the latest version. The job is not failing, it warns about not being able to compile the bridgestan model.

@sethaxen
Copy link
Contributor Author

Thanks. I don't think this is an issue with the git subtree approach. I was able to replicate it on my machine. If I set the BRIDGESTAN environment variable to point a local copy of the bridgestan repo, I still see the issue.

One thing I noticed is that the bridgestan command is pointing to a file makefile in the bridgestan repo, but only Makefile exists.

The second thing I noticed is that the command expects a .so file to be in the model directory (e.g. StanSample.jl/test/test_bernoulli/tmp), but for me none is there, which could be why the bridgestan compilation is failing.

@goedman
Copy link
Collaborator

goedman commented Oct 24, 2022

I'll take a look at the makefile/Makefile issue. Always confusing between MacOS and Linux.

On my system, in my editor (SublimeText), .so files are not visible, but if I use finder, the model files are definitely there in the tmp directory.

@goedman
Copy link
Collaborator

goedman commented Oct 24, 2022

Changed makefile to Makefile and that seems to fix the CI issue! I'll merge v6.11.2.

@sethaxen
Copy link
Contributor Author

Changed makefile to Makefile and that seems to fix the CI issue! I'll merge v6.11.2.

My 2nd issue was apparently due to missing a trailing slash in the CMDSTAN environmental variable. Once I added that, then on v6.11.2, all tests pass for me.

@goedman
Copy link
Collaborator

goedman commented Oct 25, 2022

Great! I should have pointed you to the slash issue. I also ran into that a few months ago.

One question I still have is if StanSample is installed as a package I guess the work around of defining BRIDGESTAN becomes a bit tricky ( e.g. ./StanSample/XgFgR/deps/data/bridgestan). I'll add an example/test to Stan.jl to check the above suspicion. And a test to StanSample.jl for CI testing.

But I definitely now see the power of including a git subtree! Thanks for your work and help!

@goedman
Copy link
Collaborator

goedman commented Oct 25, 2022

Will be resolved once this and this updates are merged.

@goedman
Copy link
Collaborator

goedman commented Oct 25, 2022

Hi Seth (@sethaxen), trying to create a StanSample version with the now updated bridgestan repo. Do I need to create a new version of StanSample to trigger the update of bridgestan?

By the way I really like what you are doing with PosteriorDB.jl. I've often thought about how to create some order in the many examples in Stan.jl and this might be a way forward.

@sethaxen
Copy link
Contributor Author

Hi Seth (@sethaxen), trying to create a StanSample version with the now updated bridgestan repo. Do I need to create a new version of StanSample to trigger the update of bridgestan?

You'll need to follow the instructions in https://github.com/StanJulia/StanSample.jl/blob/master/UPDATING_BRIDGESTAN.md to update the copy of bridgestan in this repo, then release a new version of StanSample.

By the way I really like what you are doing with PosteriorDB.jl. I've often thought about how to create some order in the many examples in Stan.jl and this might be a way forward.

Great! Since posteriordb is currently very Stan-focused, I anticipate most PosteriorDB users will use it in conjunction with CmdStan/StanSample, and I'd like to make sure that goes smoothly. Perhaps you have input on sethaxen/PosteriorDB.jl#1?

@goedman
Copy link
Collaborator

goedman commented Oct 25, 2022

Awesome, worked like a charm. Will release v6.11.3 this afternoon. Thanks, should have seen that!

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.

None yet

2 participants