Skip to content

feat: Encode Stan writer output using protobuf in C++#286

Merged
ahartikainen merged 9 commits intostan-dev:masterfrom
riddell-stan:feature/protobuf-shared-library
Apr 6, 2020
Merged

feat: Encode Stan writer output using protobuf in C++#286
ahartikainen merged 9 commits intostan-dev:masterfrom
riddell-stan:feature/protobuf-shared-library

Conversation

@riddell-stan
Copy link
Copy Markdown
Contributor

@riddell-stan riddell-stan commented Mar 3, 2020

Previously Python was used. Doing everything directly in C++ is much
faster and we eliminate the dependency on the Python protobuf library.

This version requires that the user install protobuf system-wide. Once
this solution is verified as working, we can remove this limitation by
copying the shared library into place.

Before this is merged:

  • Clean up C++ to eliminate the use of stringstream, most uses are unnecessary.
  • Copy the protobuf library and include files into the package, use rpath (see @riddell-stan 's example repo)

This version is about 36% faster on longer fits (lots of draws).

Closes #282
Closes #102

Comment thread .travis.yml Outdated
@riddell-stan riddell-stan marked this pull request as ready for review March 3, 2020 23:18
@riddell-stan
Copy link
Copy Markdown
Contributor Author

This is not ready to be merged.

@riddell-stan
Copy link
Copy Markdown
Contributor Author

riddell-stan commented Mar 4, 2020

@ahartikainen How do I tell gcc on Windows about a library and include directories I have installed with vcpkg? Here, in .appveyor.yml, I've used vcpkg install protobuf protobuf:x64-windows but then gcc complains about not finding the header files for protobuf.

Or is this not going to work because the shared libraries will not be compatible?

edit: add correction

@ahartikainen
Copy link
Copy Markdown
Contributor

I think lib should on the PATH

@riddell-stan
Copy link
Copy Markdown
Contributor Author

This contains a lot of useful information: microsoft/vcpkg#8577 . I'm still not sure precisely what to do. Testing is hard without a Windows computer. It seems like we're 95% of the way there.

Comment thread httpstan/models.py
Previously Python was used. Doing everything directly in C++ is much
faster and we eliminate the dependency on the Python protobuf library.

This version requires that the user install protobuf system-wide. Once
this solution is verified as working, we can remove this limitation by
copying the shared library into place.
It should be possible to get things working on Windows. With luck we can
revert this change when someone finds a solution.
@riddell-stan
Copy link
Copy Markdown
Contributor Author

@ahartikainen Have a look.

Everything important happens in the first commit, 4cd2fff. The file httpstan/callbacks_writer_parser.py is removed. The parsing work in that file is moved into C++ files, in httpstan/queue_logger.hpp and httpstan/queue_writer.hpp.

Encoding the protobuf messages in C++ means we need to link with the protobuf shared library, which we have to compile and place into the Python package (so subsequent model compilations can use it).

Copy link
Copy Markdown
Contributor

@ahartikainen ahartikainen left a comment

Choose a reason for hiding this comment

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

Looks good.

I wonder if there is a way to hack docker so it could be used for fast development?

@ahartikainen ahartikainen merged commit 7bf72da into stan-dev:master Apr 6, 2020
@riddell-stan riddell-stan deleted the feature/protobuf-shared-library branch April 22, 2020 01:16
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.

Stan source does not need to be in repo httpstan is slow with small fits (benchmarks)

2 participants