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

Operators to extend interactivity with python objects #12

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/Bonsai.Scripting.Python/Args.cs
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)
Copy link
Member

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.

{
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);
}
});
}
}
}
17 changes: 10 additions & 7 deletions src/Bonsai.Scripting.Python/EnvironmentConfig.cs
Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Up @@ -26,17 +26,14 @@ public EnvironmentConfig(string pythonHome, string pythonVersion)

public static EnvironmentConfig FromConfigFile(string configFileName)
{
var config = new EnvironmentConfig();
config.Path = System.IO.Path.GetDirectoryName(configFileName);
var config = new EnvironmentConfig
{
Path = System.IO.Path.GetDirectoryName(configFileName)
};
using var configReader = new StreamReader(File.OpenRead(configFileName));
while (!configReader.EndOfStream)
{
var line = configReader.ReadLine();
static string GetConfigValue(string line)
{
var parts = line.Split('=');
return parts.Length > 1 ? parts[1].Trim() : string.Empty;
}

if (line.StartsWith("home"))
{
Expand All @@ -59,5 +56,11 @@ static string GetConfigValue(string line)

return config;
}

private static string GetConfigValue(string line)
{
var parts = line.Split('=');
return parts.Length > 1 ? parts[1].Trim() : string.Empty;
}
}
}
40 changes: 40 additions & 0 deletions src/Bonsai.Scripting.Python/GetAttr.cs
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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are testing here the Attribute property it might be best to cache its value locally, otherwise if the property is externalized and changing dynamically, someone could set its value to null while the sequence is running and accidentally bypass this test.

{
throw new Exception("Attribute cannot be null or empty.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The framework design guidelines recommend not throwing the Exception base type. Since properties are not function arguments, for bonsai operators we have converged on using InvalidOperationException as the convention for operator state validation errors.

}
return source.Select(obj =>
{
using (Py.GIL())
{
if (!obj.HasAttr(Attribute))
{
throw new Exception($"PyObject does not have attribute {Attribute}.");
Copy link
Member

Choose a reason for hiding this comment

The 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 GetAttr would throw some kind of python exception if the attribute does not exist.

}
return obj.GetAttr(Attribute);
}
});
}
}
}
44 changes: 44 additions & 0 deletions src/Bonsai.Scripting.Python/GetItem.cs
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
Copy link
Member

Choose a reason for hiding this comment

The 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 int and string will probably be the most common indexers, can't you use other object types for indexing python objects?

Also, is there a reason why one should have precedence over the other if both are specified? For consistency with the Get and Set operators I might have turned this one around, and have PyObject be the property, while the indexer depends on the input type.

The rationale is that the PyObject is the only type which is actually fixed, while indexer types can vary with input signature.


public IObservable<PyObject> Process(IObservable<PyObject> source)
{
if (!(Index.HasValue ^ !string.IsNullOrEmpty(Key)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!(Index.HasValue ^ !string.IsNullOrEmpty(Key)))
if (Index == null && string.IsNullOrEmpty(Key))

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.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as GetAttr re. exception type and caching values for testing.

}

return source.Select(obj =>
{
using (Py.GIL())
{
if (!obj.IsIterable())
{
throw new Exception($"PyObject is not iterable.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above, I would rather avoid calling IsIterable and simply call GetItem directly.

}
return Index != null ? obj.GetItem(Index.Value) : obj.GetItem(Key);
}
});
}
}
}
27 changes: 27 additions & 0 deletions src/Bonsai.Scripting.Python/GetPythonType.cs
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())
Copy link
Member

Choose a reason for hiding this comment

The 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();
}
});
}
}
}
59 changes: 59 additions & 0 deletions src/Bonsai.Scripting.Python/Import.cs
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.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[Description("The \"as name\" of the package. For example, in \"import numpy as np\", np is the as name.")]
[Description("The name used to bind the imported package in the local scope.")]

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.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as above re. exception typing and caching. In this case ArgumentException is also not appropriate since Package is not an argument to the function call.

}
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
Copy link
Member

Choose a reason for hiding this comment

The 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 pythonnet does not automatically resolve dot-separated names, which is unfortunate.

However, it seems from this implementation that if you provide a dot-separated name you cannot use the AsName property, i.e. it will either be ignored or throw an exception related to not finding the module.

I agree it might be nice to provide semantics similar to the full import statement here, but in that case it feels like AsName should probably be applied to the last imported module, as I believe is the case in the python language.


return obj;
}
});
}
}
}
42 changes: 42 additions & 0 deletions src/Bonsai.Scripting.Python/Invoke.cs
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())
Copy link
Member

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The 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 GetAttr implicitly? That way we could invoke any kind of callable, including dynamically created functions returned from Eval, or python objects, etc.

if (!callable.IsCallable())
{
throw new Exception($"Cannot invoke callable: {callable} because it is not callable.");
Copy link
Member

Choose a reason for hiding this comment

The 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;
Copy link
Member

Choose a reason for hiding this comment

The 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);
}
});
}
}
}
43 changes: 43 additions & 0 deletions src/Bonsai.Scripting.Python/InvokeMethod.cs
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())
Copy link
Member

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having seen the pattern in both here and Invoke, I'm having a stronger feeling that PyObject should really be the property, and arguments should be the input.

}
});
}

}
}
42 changes: 42 additions & 0 deletions src/Bonsai.Scripting.Python/PyFloat.cs
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())
Copy link
Member

Choose a reason for hiding this comment

The 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 Get operator).

{
return Observable.Return(new Pythonnet.PyFloat(Value));
}
});
}

public IObservable<Pythonnet.PyFloat> Process<TSource>(IObservable<TSource> source)
{
return source.Select(obj =>
{
using (Pythonnet.Py.GIL())
Copy link
Member

Choose a reason for hiding this comment

The 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);
}
});
}
}
}
Loading