Skip to content

Add BridgeStan to user code Docker image#480

Open
simeoncarstens wants to merge 4 commits intomainfrom
simeon/use-bridgestan
Open

Add BridgeStan to user code Docker image#480
simeoncarstens wants to merge 4 commits intomainfrom
simeon/use-bridgestan

Conversation

@simeoncarstens
Copy link
Member

@simeoncarstens simeoncarstens commented Jul 19, 2023

This PR adds BridgeStan (https://github.com/roualdes/bridgestan) to the user code image. Together with a PR adding a BridgeStan wrapper to the chainsail-helpers library, this allows users to easily use BridgeStan instead of httpstan to sample from their Stan models for a ~1000x speedup 🚀

Closes #424 (although the httpstan wrapper is still available).

This is for compatibility with BridgeStan, which requires at least
Python 3.9. Note that this introduces an inconsistency with other
Python libraries / applications in Chainsail.
@simeoncarstens simeoncarstens marked this pull request as ready for review July 19, 2023 12:40
Copy link
Contributor

@dorranh dorranh left a comment

Choose a reason for hiding this comment

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

Overall this looks reasonable to me! It's a shame that BridgeStan tries to fetch dependencies at runtime, perhaps that would be worth opening as an issue upstream (e.g. to allow for an option to disable this behavior) and adding a corresponding issue here so we can keep track of it.

One question would be whether the intention is to already drop support for httpstan or if we wanted to keep it around for the time being. If the former it might be good to go ahead and drop the relevant httpstan portions of the codebase.

@simeoncarstens
Copy link
Member Author

It's a shame that BridgeStan tries to fetch dependencies at runtime, perhaps that would be worth opening as an issue upstream (e.g. to allow for an option to disable this behavior) and adding a corresponding issue here so we can keep track of it.

I'll investigate the (apparently) bug I mention in the comment in the Dockerfile more and will open an issue on BridgeStan. If that worked, then all dependencies would be available at runtime and we'd be good.

One question would be whether the intention is to already drop support for httpstan or if we wanted to keep it around for the time being. If the former it might be good to go ahead and drop the relevant httpstan portions of the codebase.

I'm not sure for now - I have tested BrigdeStan (and its wrapper in chainsail-helpers) in the user code container and it seems to work fine, but I haven't actually run a full simulation with it. If we are sure that BridgeStan stably works, then there's no reason to keep httpstan around and then we can even drop the httpstan container in the k8s node pod. I'd be happy to run more tests to reach a conclusion on this; we're not in any hurry to merge this.

@simeoncarstens
Copy link
Member Author

Re

I'll investigate the (apparently) bug I mention in the comment in the Dockerfile more and will open an issue on BridgeStan. If that worked, then all dependencies would be available at runtime and we'd be good.

Turns out it's just me being careless. 987331a works as desired 🙂

@simeoncarstens simeoncarstens requested a review from dorranh July 20, 2023 12:16
Copy link
Contributor

@dorranh dorranh left a comment

Choose a reason for hiding this comment

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

That's great that there is a way to convince BridgeStan not to fetch stanc at runtime. LGTM!

@simeoncarstens
Copy link
Member Author

@dorranh I tested this now in the full minikube deployment, too, and it works as intended (not surprising, as I had tested the user code image before already). The only thing I had to do was increase the time we wait for the user code server to be ready (the hack introduced in #382) to four minutes 😿 but it is possible that the additional time it took was related to me pulling in the BridgeStan chainsail-helpers branch via chainsail-helpers commit via git+https://github.com/tweag/chainsail-resources@simeon/bridge-stan-wrapper#subdirectory=chainsail_helpers BridgeStan takes a while to compile the model and everything. What do you think - we could either

  • put this PR on hold and address Controller tries to start sampling although nodes aren't ready yet #386 first (which would be a good idea anyways, but I'm not sure how long it would take me),
  • merge this PR and merge a PR increasing the waiting time as I did,
  • merge this PR and hope that everything is fine and the two minutes waiting time suffice once the current chainsail-helpers version is baked into the Docker image (I can test whether that makes a difference) / available on PyPi 😅

🤔

@simeoncarstens
Copy link
Member Author

I started working on fixing #386 in this draft PR: #491. We could as well just merge this (after a check that it still works, re dependencies...) and increase the waiting time.

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.

Replace httpstan wrapper with BridgeStan

2 participants