Implement remote evaluation and distributed garbage collection - #46
Implement remote evaluation and distributed garbage collection#46jonatanklosko merged 3 commits into
Conversation
Sorry, something went wrong.
|
Currently Pythonx.remote_eval/4 returns the globals map. The user may not necessarily care about all of those, so tracking those just to be GCed immediately is unnecessary work. That said, it's likely fine for the use cases where Pythonx.remote_eval/4 would be used, such as notebooks. When using Pythonx in an actual app, the evaluation should rather be behind a GenServer API, and remote objects should not be at play. If we ever discover it's a problem, we can also add an option to prune/pick the returned globals. |
Sorry, something went wrong.
| """ | ||
|
|
||
| defexception [:type, :value, :traceback] | ||
| defexception [:lines] |
There was a problem hiding this comment.
Technically it's a breaking change, though ideally I would consider the exception fields to be opaque in this case.
Sorry, something went wrong.
There was a problem hiding this comment.
FTR. I changed it so that we can easily pass Pythonx.Error across nodes without worrying about tracking and going back to the owner for formatting. I don't think storing the separate type, value, traceback objects was a good idea in the first place - if someone is interested in those, they should just have the try-catch in the Python code.
Sorry, something went wrong.
Perhaps we should make it so it will only return the last line? And if they want everything, they can call globals()? |
Sorry, something went wrong.
| if (enif_whereis_pid(caller_env, janitor_name, &janitor_pid)) { | ||
| auto device = type == 0 ? eval_info.stdout_device : eval_info.stderr_device; | ||
| // Copy the device term is from a differnet env, so we copy it into | ||
| // the message env, otherwise we may run into unexpected behaviour. |
There was a problem hiding this comment.
Which sort of unexpected behaviour?
Sorry, something went wrong.
There was a problem hiding this comment.
So far it has been working fine. Once I was testing remote eval, where stdout_device and stderr_device are remote pid terms, I run into a weird issue where the message we send below would include a random term (e.g. :infinity, {}) instead of the actual pid. I then revisited the code and realised that those terms are from a different env and doing the copy first fixed the issue. It's interesting that it's only remote pid terms that revealed the issue.
Sorry, something went wrong.
Similarly they could return a tuple with the specific values they need. However, the issue with those approach is that there is no way to return separate values to Elixir side, it would be a single %Pythonx.Object{}. And this is relevant, because in some cases they may want to do Pythonx.Decode, but only on one of those globals. |
Sorry, something went wrong.
josevalim
left a comment
There was a problem hiding this comment.
Beautiful! 😍
Sorry, something went wrong.
Ok, so we are back to square one... with the difference we would be holding fewer things in memory? |
Sorry, something went wrong.
I'm not following. My point is that if we don't return globals, it becomes an actual limitation. |
Sorry, something went wrong.
Co-authored-by: José Valim <jose.valim@gmail.com>
This adds Pythonx.remote_eval/4 to run Python code on another node. Remote eval returns "remote" %Pythonx.Object{} structs and we ensure the corresponding Python objects are kept alive on the owner node as long as necessary. There is Pythonx.copy_remote_object/1 that serializes the remote object and then deserializes it locally. Pythonx.Object also implements the FLAME.Trackable protocol, which sets up proper lifetime tracking for objects returned from FLAME.call/3 (similarly to Pythonx.remote_eval/4.
More details on the API in Pythonx docs, and implementation details in Pythonx.ObjectTracker.
We could have a separate Pythonx.Remote module, but the API surface is small, so I am not sure if we necessarily need that.