-
Notifications
You must be signed in to change notification settings - Fork 5
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
Operators to extend interactivity with python objects #12
base: main
Are you sure you want to change the base?
Changes from all commits
7d01c3c
c73049b
db1ed53
8e3f362
fc825fb
33a430d
05747ba
3418573
7bb55d7
34ca198
d6f49ea
6dd7f65
9deb720
f3febf9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
using System; | ||
using System.Collections; | ||
using System.Reactive.Linq; | ||
using Pythonnet = Python.Runtime; | ||
using System.Linq; | ||
using System.Runtime.CompilerServices; | ||
using System.Reflection; | ||
|
||
namespace Bonsai.Scripting.Python | ||
{ | ||
/// <summary> | ||
/// Represents an operator that takes a sequence of python objects and converts them to a type of PyTuple to pass as arguments to a function. | ||
/// </summary> | ||
[Combinator] | ||
[WorkflowElementCategory(ElementCategory.Transform)] | ||
public class Args | ||
{ | ||
public IObservable<Pythonnet.PyTuple> Process(IObservable<object> source) | ||
{ | ||
return source.Select(obj => | ||
{ | ||
using (Pythonnet.Py.GIL()) | ||
{ | ||
if (!(obj is ITuple || obj is IList || obj is Array)) | ||
{ | ||
if (obj is Pythonnet.PyObject pyObj) | ||
{ | ||
return new Pythonnet.PyTuple(new Pythonnet.PyObject[] {pyObj}); | ||
} | ||
|
||
return new Pythonnet.PyTuple(new Pythonnet.PyObject[] {Pythonnet.PyObject.FromManagedObject(obj)}); | ||
} | ||
|
||
PropertyInfo[] properties = obj.GetType().GetProperties(); | ||
Pythonnet.PyObject[] pyObjects = new Pythonnet.PyObject[properties.Length]; | ||
|
||
for (int i = 0; i < properties.Length; i++) | ||
{ | ||
object value = properties[i].GetValue(obj, null); | ||
|
||
if (!(value is Pythonnet.PyObject)) | ||
{ | ||
throw new ArgumentException($"All elements of the tuple must be of type PyObject. Instead, found {value.GetType()} for Item{i+1}."); | ||
} | ||
|
||
pyObjects[i] = (Pythonnet.PyObject)value; | ||
} | ||
return new Pythonnet.PyTuple(pyObjects); | ||
} | ||
}); | ||
} | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these changes required? They seem unrelated to the new operators, but I wonder whether you may have had trouble compiling in Linux perhaps? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
using System; | ||
using System.ComponentModel; | ||
using System.IO; | ||
using System.Reactive.Linq; | ||
using Python.Runtime; | ||
using System.Xml.Serialization; | ||
using System.Linq; | ||
|
||
namespace Bonsai.Scripting.Python | ||
{ | ||
/// <summary> | ||
/// Represents an operator that gets an attribute from a python object if the attribute exists. | ||
/// </summary> | ||
[Combinator] | ||
[WorkflowElementCategory(ElementCategory.Transform)] | ||
public class GetAttr | ||
{ | ||
[Description("The name of the attribute to get.")] | ||
public string Attribute { get; set; } | ||
|
||
public IObservable<PyObject> Process(IObservable<PyObject> source) | ||
{ | ||
if (string.IsNullOrEmpty(Attribute)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you are testing here the |
||
{ | ||
throw new Exception("Attribute cannot be null or empty."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The framework design guidelines recommend not throwing the |
||
} | ||
return source.Select(obj => | ||
{ | ||
using (Py.GIL()) | ||
{ | ||
if (!obj.HasAttr(Attribute)) | ||
{ | ||
throw new Exception($"PyObject does not have attribute {Attribute}."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to check and throw here? I would expect that simply calling |
||
} | ||
return obj.GetAttr(Attribute); | ||
} | ||
}); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,44 @@ | ||||||
using System; | ||||||
using System.ComponentModel; | ||||||
using System.IO; | ||||||
using System.Reactive.Linq; | ||||||
using Python.Runtime; | ||||||
using System.Xml.Serialization; | ||||||
using System.Linq; | ||||||
|
||||||
namespace Bonsai.Scripting.Python | ||||||
{ | ||||||
/// <summary> | ||||||
/// Represents an operator that gets an item from a generic python iterable using either a numbered index, or a string key. | ||||||
/// </summary> | ||||||
[Combinator] | ||||||
[WorkflowElementCategory(ElementCategory.Transform)] | ||||||
public class GetItem | ||||||
{ | ||||||
[Description("The index to get.")] | ||||||
public int? Index { get; set; } = null; | ||||||
|
||||||
[Description("The key to get.")] | ||||||
public string Key { get; set; } = null; | ||||||
Comment on lines
+19
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the current implementation, I'm wondering whether this is the best design. While indeed Also, is there a reason why one should have precedence over the other if both are specified? For consistency with the The rationale is that the |
||||||
|
||||||
public IObservable<PyObject> Process(IObservable<PyObject> source) | ||||||
{ | ||||||
if (!(Index.HasValue ^ !string.IsNullOrEmpty(Key))) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Is there a particular reason to use the XOR operator here? It is unusual to use it in logical expressions, and if I understand the reasoning correctly the suggestion above might be more readable. |
||||||
{ | ||||||
throw new Exception("Either an index or a key must be provided."); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comments as |
||||||
} | ||||||
|
||||||
return source.Select(obj => | ||||||
{ | ||||||
using (Py.GIL()) | ||||||
{ | ||||||
if (!obj.IsIterable()) | ||||||
{ | ||||||
throw new Exception($"PyObject is not iterable."); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above, I would rather avoid calling |
||||||
} | ||||||
return Index != null ? obj.GetItem(Index.Value) : obj.GetItem(Key); | ||||||
} | ||||||
}); | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
using System; | ||
using System.Reactive.Linq; | ||
using Python.Runtime; | ||
using System.Linq; | ||
|
||
namespace Bonsai.Scripting.Python | ||
{ | ||
/// <summary> | ||
/// Represents an operator that gets the type of a python object. Equivalent to calling type(obj) in python. | ||
/// </summary> | ||
[Combinator] | ||
[WorkflowElementCategory(ElementCategory.Transform)] | ||
public class GetPythonType | ||
{ | ||
|
||
public IObservable<PyObject> Process(IObservable<PyObject> source) | ||
{ | ||
return source.Select(obj => | ||
{ | ||
using (Py.GIL()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency with the remaining operators and the new GIL logic, we should probably remove the implicit locking here. |
||
{ | ||
return obj.GetPythonType(); | ||
} | ||
}); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,59 @@ | ||||||
using System; | ||||||
using System.ComponentModel; | ||||||
using System.IO; | ||||||
using System.Reactive.Linq; | ||||||
using Python.Runtime; | ||||||
using System.Xml.Serialization; | ||||||
using System.Linq; | ||||||
|
||||||
namespace Bonsai.Scripting.Python | ||||||
{ | ||||||
/// <summary> | ||||||
/// Represents an operator that will import the name of a package as a user-defined named package into the input module. | ||||||
/// </summary> | ||||||
[Combinator] | ||||||
[WorkflowElementCategory(ElementCategory.Transform)] | ||||||
public class Import | ||||||
{ | ||||||
[Description("The name of the package.")] | ||||||
public string Package { get; set; } | ||||||
|
||||||
[Description("The \"as name\" of the package. For example, in \"import numpy as np\", np is the as name.")] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I would avoid giving a specific example here, or if absolutely necessary I would use a standard library module, for future proofing the documentation. |
||||||
public string AsName { get; set; } | ||||||
|
||||||
public IObservable<PyObject> Process(IObservable<PyModule> source) | ||||||
{ | ||||||
if (string.IsNullOrEmpty(Package)) | ||||||
{ | ||||||
throw new ArgumentException(nameof(Package), "A package must be specified."); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comments as above re. exception typing and caching. In this case |
||||||
} | ||||||
return source.Select(module => | ||||||
{ | ||||||
using (Py.GIL()) | ||||||
{ | ||||||
if (!string.IsNullOrEmpty(AsName)) | ||||||
{ | ||||||
return module.Import(Package, AsName); | ||||||
} | ||||||
|
||||||
if (!Package.Contains('.')) | ||||||
{ | ||||||
return module.Import(Package); | ||||||
} | ||||||
|
||||||
var packages = Package.Split('.'); | ||||||
var packagePath = string.Empty; | ||||||
|
||||||
PyObject obj = null; | ||||||
for (int i = 0; i < packages.Length; i++) | ||||||
{ | ||||||
packagePath = string.IsNullOrEmpty(packagePath) ? packages[i] : $"{packagePath}.{packages[i]}"; | ||||||
obj = module.Import(packagePath); | ||||||
} | ||||||
Comment on lines
+44
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am unsure about the intended semantics here. Given the code I am assuming that However, it seems from this implementation that if you provide a dot-separated name you cannot use the I agree it might be nice to provide semantics similar to the full import statement here, but in that case it feels like |
||||||
|
||||||
return obj; | ||||||
} | ||||||
}); | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
using System; | ||
using System.ComponentModel; | ||
using System.IO; | ||
using System.Reactive.Linq; | ||
using Pythonnet = Python.Runtime; | ||
using System.Xml.Serialization; | ||
using System.Linq; | ||
|
||
namespace Bonsai.Scripting.Python | ||
{ | ||
/// <summary> | ||
/// Represents an operator that will invoke a callable object and pass the given arguments to the call. The input can either be the callable object itself, or can be an object which has a named callable attribute. | ||
/// </summary> | ||
[Combinator] | ||
[WorkflowElementCategory(ElementCategory.Transform)] | ||
public class Invoke | ||
{ | ||
[Description("The name of the callable object to invoke.")] | ||
public string Callable { get; set; } | ||
|
||
[XmlIgnore] | ||
[Description("The args to pass to the callable.")] | ||
public Pythonnet.PyTuple Args { get; set; } = null; | ||
|
||
public IObservable<Pythonnet.PyObject> Process(IObservable<Pythonnet.PyObject> source) | ||
{ | ||
return source.Select(obj => | ||
{ | ||
using (Pythonnet.Py.GIL()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably remove the implicit locking for consistency with the new GIL approach. |
||
{ | ||
var callable = string.IsNullOrEmpty(Callable) ? obj : obj.GetAttr(Callable); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency, shouldn't we just provide the callable as an object property instead of using |
||
if (!callable.IsCallable()) | ||
{ | ||
throw new Exception($"Cannot invoke callable: {callable} because it is not callable."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above regarding exception typing. |
||
} | ||
var args = Args == null ? new Pythonnet.PyTuple() : Args; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can probably use the null-coalescing operator to avoid the property double-access here. We should probably also cache the property value at observable creation time to avoid race conditions on parallel workflows. |
||
return callable.Invoke(args); | ||
} | ||
}); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
using System; | ||
using System.ComponentModel; | ||
using System.IO; | ||
using System.Reactive.Linq; | ||
using Pythonnet = Python.Runtime; | ||
using System.Xml.Serialization; | ||
using System.Linq; | ||
|
||
namespace Bonsai.Scripting.Python | ||
{ | ||
/// <summary> | ||
/// Represents an operator that will invoke a method of the object and pass the given arguments to the method. | ||
/// </summary> | ||
[Combinator] | ||
[WorkflowElementCategory(ElementCategory.Transform)] | ||
public class InvokeMethod | ||
{ | ||
[Description("The name of the method to invoke.")] | ||
public string Method { get; set; } | ||
|
||
[XmlIgnore] | ||
[Description("The args to pass to the callable.")] | ||
public Pythonnet.PyTuple Args { get; set; } = null; | ||
|
||
public IObservable<Pythonnet.PyObject> Process(IObservable<Pythonnet.PyObject> source) | ||
{ | ||
if (string.IsNullOrEmpty(Method)) | ||
{ | ||
throw new Exception("Method name cannot be null or empty."); | ||
} | ||
|
||
return source.Select(obj => | ||
{ | ||
using (Pythonnet.Py.GIL()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the GIL for consistency with the new approach. |
||
{ | ||
var args = Args == null ? new Pythonnet.PyTuple() : Args; | ||
return obj.InvokeMethod(Method, args); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having seen the pattern in both here and |
||
} | ||
}); | ||
} | ||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
using System; | ||
using System.ComponentModel; | ||
using System.Reactive.Linq; | ||
using Pythonnet = Python.Runtime; | ||
using System.Linq; | ||
|
||
namespace Bonsai.Scripting.Python | ||
{ | ||
/// <summary> | ||
/// Represents the creation of a float python data type. | ||
/// </summary> | ||
[Combinator] | ||
[WorkflowElementCategory(ElementCategory.Source)] | ||
public class PyFloat | ||
{ | ||
|
||
[Description("The value of the float.")] | ||
public double Value { get; set; } | ||
|
||
public IObservable<Pythonnet.PyFloat> Process() | ||
{ | ||
return Observable.Defer(() => | ||
{ | ||
using (Pythonnet.Py.GIL()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case it is probably OK to keep the GIL, but we should probably wait for the runtime to be initialized to avoid access violation exceptions (see implementation for the |
||
{ | ||
return Observable.Return(new Pythonnet.PyFloat(Value)); | ||
} | ||
}); | ||
} | ||
|
||
public IObservable<Pythonnet.PyFloat> Process<TSource>(IObservable<TSource> source) | ||
{ | ||
return source.Select(obj => | ||
{ | ||
using (Pythonnet.Py.GIL()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I would remove the GIL for consistency with the new locking approach. |
||
{ | ||
return new Pythonnet.PyFloat(Value); | ||
} | ||
}); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would both be more idiomatic and performant here to have strongly-typed overloads for compatible types, or implement the operator using a full-blown
ExpressionBuilder
.This way we wouldn't have to rely on dynamic type checking and reflection to know how to construct the
PyTuple
object, since we would know the exact types at compile time. We could then automatically generate code that builds the tuple for the exact type signature and skip all the expensive reflection calls altogether.