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

Official docker image #239

Open
zwimer opened this issue Oct 12, 2022 · 7 comments
Open

Official docker image #239

zwimer opened this issue Oct 12, 2022 · 7 comments

Comments

@zwimer
Copy link
Contributor

zwimer commented Oct 12, 2022

Would you be open to a PR that auto-publishes a docker build of this to with the image tag gchr.io/RosettaCommons/binder on updates to master?

Right now I'm manually maintaining zwimer/binder; though I rebuilt this every time master is updated.

What I could do is add a github workflows YML to automatically do this. Pushing to gchr.io/RosettaCommons/binder is easy as that is github's docker repo. Though, I could easily make this work for a dockerhub ID (ie just the tag RosettaCommons/binder) if you wanted to add 2 github secrets, DOCKERHUB_USERNAME and DOCKERHUB_TOKEN; these would be protected by the github secrets mechanisms.

Here is an example yml file I could make the PR using (though I would slightly modify it): https://github.com/zwimer/caddy-cloudflare/blob/master/.github/workflows/publish.yml

@lyskov
Copy link
Member

lyskov commented Oct 13, 2022

I am not sure how useful this would be for Binder users, could you please elaborate use case that you have in mind? My main concern is that code generated by Binder is depended on C++ std version. Because of that for my projects i always have to build Binder and generate code on target systems because trying to generate bindings on one system and compile it on other leads to various compilation issues for non-trivial cases . And since we already providing "reference" Docker file it should be easy for users to adapt it for they particular case.

@zwimer
Copy link
Contributor Author

zwimer commented Oct 14, 2022

Interesting. I have not encountered an issue using binder to generate code for one OS vs another; most of what binder generates seem to be just standard C++ interacting with the pybind11 header files; which translate (in my limited experience) just fine between (for example) MacOS and various flavors of Linux across both clang++ and g++.

Would it be possible to provide an example of what binder has issues with? It didn't seem (from my very limited poking around of the source code) to bind anything distro-specific; i.e. it generated vanilla C++ that should just work across various Linux distros.

If I misunderstood and you meant you need to be able to control the flags given to binder while binding: the docker image should support this just fine. I personally use the reference Dockerfile and just invoke it as:

#!/bin/bash -eux
docker run --rm \
	--user "$(id -u):$(id -g)" \
	`# Mount in everything` \
	-v "$(pwd):/in:ro" \
	-v "$(pwd)/out:/out" \
 	-it "zwimer/binder" \
	`# Run binder` \
	/usr/local/bin/binder \
		--config="/in/a.conf"  \
		-v \
		--root-module foo \
		--prefix "/out" \
		"/in/include.hpp" \
		-- \
		-std=c++17		

^ Notice I'm still run-time configuring the proper std version in the last line here.

I do this because I have had difficulty building binder with llvm on mac before and this was a really quick and easy work-around that required 0 effort debugging on my end.

Personally, I had trouble compiling binder myself on M1, especially when I first started using the tool; perhaps due to my own ignorance but also likely due to the documentation using LLVM 6 (15 is current). If I recall correctly, since then LLVM has changed such that these instructions don't work and I have to fumble my way around this for a while before eventually giving up and doing this on Linux. Though since I needed to do it on mac I eventually just made the docker image.

It is possible this is more of a personal problem, I just see this as a solution that might functionally remove the need to stress over this whole page for some users.

@lyskov
Copy link
Member

lyskov commented Oct 15, 2022

re issues when building on M1: - thank you for letting me know! I just pushed patch that should fix this (i am assuming that you are using provided build.py scripts, - if not then just build Binder using LLVM-13)

re generated bindings difference: - could it be that you are building with --include-pybind11-stl flag? - the differences will starting to surface when Binder tries to bind C++ std classes, particularly templates. Main reason that most std implementations will use typedef's/type-aliases now and then. In such cases Binder will always generate code using 'real' underlying classes instead of typedefs and so such code could only be compiled with the same stl implementation/version.

@zwimer
Copy link
Contributor Author

zwimer commented Oct 15, 2022

Ah, yes I am using that flag; your reasoning makes sense in that case.

Given this uses LLVM, would do you imagine it would be a 'quick fix' to have binder use the typedefs rather than the underlying classes? I think this would also make the output generally more read-able. If it is short enough I might be able to PR it, time-permitting.

@lyskov
Copy link
Member

lyskov commented Oct 27, 2022

Unfortunately making Binder using the typedefs/aliases is not as simple as it might look. I have considered this approach a few times in the past and its boils down to issue that there could be multiple typedefs for single types and to make situation even more complicated we have templated aliases which makes mapping even more complicated. The more promising path (and this is what i am currently considering) is to make standard_name (https://github.com/RosettaCommons/binder/blob/master/source/type.cpp#L507) much more robust and try to make it work in 100% of the cases on supported platforms. However there is many versions of C++ STD lib and also this code is somewhat performance critical.

@zwimer
Copy link
Contributor Author

zwimer commented Oct 27, 2022

Darn, I would have assumed clang would have a way for you to get the name of the type / alias, as it was when parsed, rather than just the underlying type. That's unfortunate.

@andriish
Copy link
Contributor

andriish commented Nov 7, 2022

Hi @zwimer ,

just if you want a solution for Mac, binder is packaged here
https://github.com/davidchall/homebrew-hep

brew tap davidchall/hep
brew install binder

and here for Fedora/CentOS -- here
https://copr.fedorainfracloud.org/coprs/averbyts/HEPrpms/

 dnf -y copr enable averbyts/HEPrpms 
 dnf -y install binder

And it is trivial to create a docker image from the latter.

Best regards,

Andrii

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

No branches or pull requests

3 participants