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

major rewrite to work with current Go versions #180

Closed
wants to merge 70 commits into from

Conversation

rcoreilly
Copy link
Member

This is a major rewrite, using the idea of int64 handles instead of passing pointers around in cgo / python. It all works, and along the way lots of additional functionality added to be able to handle full multi-package systems, and even generate a standalone python executable as was needed for my GoGi gui system where the main thread is taken up by the gui event loop.

tests all pass for py3 but py2 has some different output sequencing so produces diff outputs sometimes.

I also switched over to using pybindgen for speed on cpython (and ease of use) -- cffi should be possible and not too hard (hopefully just changing the generating of the build.py script). It handles all the lower-level glue so the cgo wrappers and .py wrappers all look very straightforward. Switched to more go-standard camel-case, shorter naming style instead of all the kebab-case cgo names that I found hard to parse.. generated code seems pretty readable to me now.

.py wrappers support full container features on maps and slices. constants are just directly declared in .py wrapper -- no going through go. arrays are treated as go-generated only given their fixed size -- use slices for any py-side created lists.

overall, the translation for a given .go file to python do the same thing with the wrapped code is remarkably similar and illustrates the great fit between the two languages -- go is a nice subset of python and using go for fast backend computation makes a lot of sense. see the GoGi demo case linked on the updated README.md for an example.

Randall C. O'Reilly added 30 commits February 11, 2019 03:47
…ncodes type name along with counter and does type-safe checks so it won't fail on the conversion. bind command builds using pybindgen.
… always need to convert back and forth to / from pointer for handle
…and now can switch to int64 instead of string keys as py wrapper makes string less important.
… multiple packages easily in python -- look into install etc
…y; interface methods don't create symbols, but turns out nil obj is ok..
… figured out how to navigate python packages finally..
…le registry being diff for each .so compiled library: need to now make mega-libraries.
… -- computed simply. everything always scoped by pkg name in prep for integrated multi-package output.
…go.nil. all working for emergent/leabra python script.
…d in order, so now need to sort the pywrap struct output so embedded are always earlier..
…ild. use Embed method on all struct method calls to get proper virtual function calling.
@rcoreilly
Copy link
Member Author

I guess I can look into this -- some issues with the environment on travis:

# pkg-config --cflags  -- /opt/pyenv/lib/pkgconfig/python3.pc
320  Failed to open '/opt/pyenv/lib/pkgconfig/python3.pc': No such file or directory
321  No package '/opt/pyenv/lib/pkgconfig/python3.pc' found
322  pkg-config: exit status 1

this does not look promising: travis-ci/travis-ci#8217

I'm currently moving so may not be able to do this for a few days.

@sbinet
Copy link
Member

sbinet commented Aug 1, 2019

yeah, that's why (and also b/c of Windows) there's this getPythonConfig function in bind/util.go.
see:

perhaps it would need to be refactored into its own package so it could be used from cmd_build.go ?

I'm currently moving so may not be able to do this for a few days.

no worries. (I've just emerged from my email backlog of a 3 weeks holidays...)

@rcoreilly
Copy link
Member Author

Umm.. it looks like it is now just working!?

https://travis-ci.org/go-python/gopy

@sbinet
Copy link
Member

sbinet commented Aug 11, 2019

that's master :)

this is my latest attempt: https://travis-ci.org/go-python/gopy/builds/566516723

@rcoreilly
Copy link
Member Author

now that this is all in a branch (pr-180) I don't seem to have push access.. In testing on my linux VM, I was able to replicate the error, and fixed it by setting the LD_LIBRARY_PATH to include current dir : . So adding this after the PATH setting in .travis.yml should hopefully fix it:

   # ld needs to look in current directory to load the _go.so library
   - export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:.

we presumably need to add this to the README.md as well, for anyone running on linux without this set. somehow mac defaults to using . automatically, so it didn't come up there.

@sbinet
Copy link
Member

sbinet commented Aug 12, 2019

thanks for tackling this (and your persistance).

it's better:

but there are still a couple of failures (some are what seems to be stdout buffering issues, others are float/int divisions)

the pr-180 branch was meant to be cherry-picked from, so you could incorporate my attempts into your branch.

eg:

$> git checkout your-branch
$> git cherry-pick 9a6a124 a74d9d3 7fa5a17 74d5313 9dc7486 e637039 525b305 13b2ccf 1df5848 008dc92 fb6e893 de9829a 

hth,
-s

@rcoreilly
Copy link
Member Author

ok. we will need different "want" outputs for py2 vs. py3 :(

@rcoreilly
Copy link
Member Author

I have no idea what this conflict thing is about -- I just edited .travis.yml trivially to resolve, but now it is causing merge problems -- must have been some kind of whitespace? hopefully you can resolve at final merge?

also, it looks like mac vs. linux has different output -- tests all pass (except v2 vs. v3 diffs) on my mac but not on linux -- I don't suppose you have any experience with that? should we just target linux given that is what runs on travis?

@rcoreilly
Copy link
Member Author

I verified that "go test" passes all tests on linux when only using py3, and filed an issue for adding separate want target for py2. If this works on travis, then it would be great to just merge this thing and do additional work from there. It looks like something bad is happening with this conflict (or something else) that is now preventing travis from running:

https://travis-ci.org/go-python/gopy/requests

there is also a possibility that the environment on travis works differently than my "go test" run on my linux vm, in which case we have another problem..

@justinfx
Copy link
Contributor

justinfx commented Aug 13, 2019

I've been tracking the activity in this pr-180 branch and decided to give the current state a test in my project. My project currently has been using a hand-rolled combination of github.com/sbinet/go-python to generated a c-shared library and ctypes to expose the 3 function it needs for passing *PyObject in and getting a *PyObject back. So to test this pr-180 branch, I split out the main.go into a new packaged without the export go-python binding stuff. I'm left with 3 public functions that I expect to export to python.
Now I have a couple questions about the results I am getting...

Building using python-2.7, with gopy pkg -name='mypkg' domain.com/group/proj/cmds/gopytest

It seems a bit fiddly with regards to modules vs $GOPATH. I had to disable GO111MODULE and make sure gopy and my project were in $GOPATH so it could find everything.

The LDFLAGS don't seems to include something like -Wl,-rpath=$ORIGIN which means when I try to import the generated pkg, the _mypkg.so can not find mypkg_go.so. So I hand edited the Makefile to add -Wl,-rpath='$$ORIGIN' and re-run make build. Should there be something like this by default or at least a way to inject linker flags?

My project defines quite a lot of packages and the cmds/gopytest/main.go has quite a big import chain, so I was pretty impressed to find that I was able to make a successful call through the generated module right off the bat:

  1. Construct a "Request" class (struct) in py
  2. Pass it to the Resolve function in my module
  3. Get back a "Result" class (struct) type

Now my question is about the fields that are part of the result.

type Result struct {
    Things []*api.Thing
}

I found that my Result.Things field did not directly contain the wrapped Thing instances, but rather a slice Slice_Ptr_api_Thing([2157, 2158, .... I was able to manually map them with something like [mypkg.Thing(handle=i) for i in result.Things]. Is that expected? Or ideally would they already be wrapped instead of being a list of long ids?

Also I found that the api.Thing type was not exported automatically from just being imported and used in the public function. I had to type alias it into the cmds/gopytest/main.go file to get it picked up by gopy and exported as a class in python.

Really interested in this branch, and thanks for all the work!

@rcoreilly
Copy link
Member Author

rcoreilly commented Aug 14, 2019

@justinfx -- you can specify multiple packages to process (edit: directly in the gopy args), and you might get better results by including the api package directly -- that might cause it to properly interpret the handles as *api.Thing instead of having them as bare handles. If not, could you file a ticket? It doesn't look like our slice test case tests slices of structs or *struct so we should add that. I would prefer to do that after the merge instead of adding more to this current pr which seems to be getting a bit unwieldy.

I'm pretty sure most of my current real-world use cases are slices of interface types and those all seem to be working properly without any bare handles being exposed.

Also, adding an additional LDFLAGS would be a good idea. I'm not sure if the current cgo versions of those env vars might already work? look that up and give it a try if you didn't already.

@justinfx
Copy link
Contributor

I ended up creating a small test project and submitting #191 to illustrate the problem with the multiple packages and slice of structs.

I'm pretty sure most of my current real-world use cases are slices of interface types and those all seem to be working properly without any bare handles being exposed.

Per #191 I tried interfaces but still saw handles :-/

Also, adding an additional LDFLAGS would be a good idea. I'm not sure if the current cgo versions of those env vars might already work? look that up and give it a try if you didn't already.

Setting CGO_LD_FLAGS only influences the linking of "package_go.so" whereas we need to influence the flags for "_package.so". Those compiler flags seem to be derived from the pkg-config python flags. So there would need to be a way to pass more flags into that compiler/linker command that gets built by gopy.

@sbinet
Copy link
Member

sbinet commented Aug 23, 2019

oops, I tried to fixup the merge conflicts using the github web editor but it seems it committed my changes into your goki/gopy:master branch.
that's not very polite. sincere apologies.

@sbinet sbinet mentioned this pull request Aug 23, 2019
@sbinet sbinet closed this in #192 Aug 24, 2019
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.

5 participants