go-python / gpython

gpython is a python interpreter written in go "batteries not included"
BSD 3-Clause "New" or "Revised" License
869 stars 95 forks source link

Multi-context execution (py.Context) #144

Closed drew-512 closed 2 years ago

drew-512 commented 2 years ago

Hi Seb, it's a been a couple months but things are well here.

With this PR, I am pleased to share gpython with multi-context execution! Check out type Context interface and vm/vm_test.go for a quick start.

My intention with this PR is get feedback from you on what you want me to alter or massage so that you are satisfied. I am totally open if you want me to shift, rename, or organize code in ways that you would prefer, so kindly please just let me know. I think many will be highly interested in a multi-ctx gpython (as many have asked for it as you already know).

So if you like things as they are, you can merge this PR. Otherwise, just give me a list of things you want changed and I'll resubmit a new PR with that. Or, perhaps merge this PR as is, insert edits as you see fit, tag it into a new release and version, and then I will follow your lead and ditch my fork and we can be on our way. :)

We are definitely using gpython here for our classified particle research project and would love to join forces with you on maintaining gpython. I think it is highly reasonable, especially with this PR, that gpython is picked up (sponsored) since it serves the Google, Python, and Go communities quite uniquely.

drew-512 commented 2 years ago

Liking the your CI scripts!

Added two more commits that addressed those tests not passing

sbinet commented 2 years ago

thanks. it's quite a large PR :) I'll try to digest it over the coming days (with probably a hiatus Tue-Wed).

drew-512 commented 2 years ago

Hi friend. You'll see that much of the code is just additional helper/utility code (as well as the couple inner core changes relating to py.Ctx). I am open on any renaming or code reorganization that calls to you. I would like to build trust with you and continue to grow gpython, so please let me know on what flow and communication modes you prefer. If you are feeling ambitious, the code now is in a position for us to put better import code in or many other kinds of things that people want. A multi-ctx python in Go will be fo interest to many and I think gpython has major potential, as it bridges the gap between devs like us plugged into the power of Go and the kind of people we make software for (scientists and researchers and UI devs who are best with Python for their work).

In the coming months, I will be using gypthon to power a Go-based UI runtime that "serves" a Unity client UI. The Unity client runs a 2D physics-based UI and embeds the Go process. This will power the UI for this particle research suite we are developing...

sbinet commented 2 years ago

also, it would be great to have an idea of the loss (probably?) of performances wrt "single context" execution.

drew-512 commented 2 years ago

also, it would be great to have an idea of the loss (probably?) of performances wrt "single context" execution.

Please check out examples/multi-ctx.go and tell me what you think? What do you suggest I do from here -- I am totally open to ideas and don't have a lot of experience w/ Go's test/bench features.

Other than the py/function.go issue still open for resolution above and how this multi-ctx demo, are you seeing anything else my friend?

If you think I should make a concrete py.Ctx (struct) rather than an interface, I am open to that. Is your basis for this because there is less overhead?

I tried reverted everything to 644, sorry about there. There were a few scripts remaining and I wasn't sure what made sense there.

Best, Drew

drew-512 commented 2 years ago

I'm not understanding what is happening with the 644 issue. I checkout a new copy of gpython and I see 755 everywhere, so I'm not clear what I should be doing or missed, sorry. Please let me know what I can do, thanks.

sbinet commented 2 years ago

could you rebase off the latest master?

(there's a little conflict in go.mod as I have bumped the minimum Go version to 1.16)

drew-512 commented 2 years ago

Hi @sbinet , excited to be supporting you on all this and thanks for the catches and suggestions.

As I mentioned in the other thread, I'll update the README for your review in the coming days.

Also, one idea I wanted to explore was renaming py.Ctx to py.Context? Either seem ok or perhaps you have another. I defer to your experience friend.

Drew

drew-512 commented 2 years ago

Good suggestions in your opening comment and will do a revision.

sbinet commented 2 years ago

s/py.Ctx/py.Context/g is fine by me.

codecov[bot] commented 2 years ago

Codecov Report

Merging #144 (2b7e1ab) into master (965bc08) will decrease coverage by 0.91%. The diff coverage is 52.61%.

:exclamation: Current head 2b7e1ab differs from pull request most recent head 02d6b3e. Consider uploading reports for the commit 02d6b3e to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
- Coverage   74.54%   73.63%   -0.92%     
==========================================
  Files          64       67       +3     
  Lines       11182    11512     +330     
==========================================
+ Hits         8336     8477     +141     
- Misses       2296     2468     +172     
- Partials      550      567      +17     
Impacted Files Coverage Δ
py/code.go 11.90% <0.00%> (ø)
py/float.go 53.17% <0.00%> (-0.31%) :arrow_down:
py/range.go 86.30% <ø> (-0.10%) :arrow_down:
py/traceback.go 22.22% <ø> (-2.11%) :arrow_down:
py/util.go 0.00% <0.00%> (ø)
py/import.go 15.62% <45.16%> (-0.86%) :arrow_down:
modules/runtime.go 45.86% <45.86%> (ø)
py/bytes.go 28.07% <50.00%> (ø)
py/method.go 70.52% <50.00%> (ø)
py/run.go 60.46% <60.46%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 965bc08...02d6b3e. Read the comment docs.

drew-512 commented 2 years ago

Love all the CI stuff you're adding, awesome!

The items I'm working on in the coming days are a README rev, a well documented embedded example (huge for newcomers), and the single vs multi ctx benchmark as you suggested. I think gpython as a viable, reliable embedded interpreter solution has very high potential. Something as potent, easy, and fast to embed as Lua, but in general, devs don't ever want to choose that b/c they don't want to tell their users to have to learn {{$esoteric_embedded_language}}. My vision is that RunFile(), multi-context, and this embedding example are inviting and positive signs for those evaluating gpython for serious use.

I've heard many big cats often quip "the second most popular language in the world", reflecting that when it comes to non-developers (finance, science), Python has the best learning curve and those users typically already have some python experience. Also, many of those types want to grow their python skills b/c they see it show up all over their career. So I suspect this could be a big theme, now we are in world awash in interpreters that are basically only interesting to professional developers. The gpython multi-context additions for this physics research group I mentioned are working out very well and seem to "complete" gpython for real world Go applications.

I would love to remind you and @ncw what a huge freaking achievement running any python 3 in Go is, and that gpython's potential is not to be underestimated as a leading Go embedding option. Or perhaps I see no reason why Google would not want to donate a summer's worth of resources to gpython if that is something beneficial to gpython.

Drew

drew-512 commented 2 years ago

Indeed! :D

Good suggestions, btw.

Commits coming in by the end of this weekend is a documented, real-world embedding example, readme edits, and that benchmark.

drew-512 commented 2 years ago

I'll switch to do and commit that benchmark first -- just had been putting that off, waaah.

drew-512 commented 2 years ago

@sbinet for the pre-context vs with-context benchmarks you raised, I'm not clear on what the path is. I am getting that you want a PR that does bench X on gpython before py.Context and then see how the test performs with the py.Context applied.

Questions:

1) Is the intention/purpose to just see if there's a load penalty of an extra interface{} traveling around everywhere? I mean, I can't see how it would be anything but negligible, especially when you look at how much GC load there is from any kind of execution. Really, the only change is that instead of going to a global module lookup, the lookup occurs on a context-instanced map which of course is lateral.

2) Because thepy.Context is just another field in the VM struct, I'm not even sure how any python code can/would test any performance hit, other than, say, comparing the pystone of pi_chudnovsky_bs.py before and after the py.Context adds. In your prev post, you mentioned a "local" variable edit -- do you mean embedded callbacks or editing a module local?

Thanks.

drew-512 commented 2 years ago

Hi everyone!

At this point I am holding off on any new work on my fork and awaiting guidance on anything further. I am happy to take feedback and @sbinet please let me know on my questions above regarding bench comparison. In summary: analysis of the py.Context shows, at most, the penalty can't even be measured.

Aside from readme or embedded example edits, I recommend we merge and celebrate this exciting step forward. A lot of people will find this multi-context capability exciting and recognize what it unlocks!

drew-512 commented 2 years ago

Big thanks to everyone has helped make gpython happen over the years -- though it has been some time!

@ncw it would be great to get any feedback or your blessing on any of the code or the README.

Otherwise, @sbinet has been making great suggestions and I think we're in great shape to almost ready to merge! 🎉

sbinet commented 2 years ago

superseded by #158.