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

Refactor python engines api #14940

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 6 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
2 changes: 1 addition & 1 deletion src/DynamoPackages/PackageLoader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ private void TryLoadPackageIntoLibrary(Package package)


PythonServices.PythonEngineManager.Instance.
LoadPythonEngine(package.LoadedAssemblies.Select(x => x.Assembly));
LoadPythonEngine(Log, package.LoadedAssemblies.Select(x => x.Assembly));

Log($"Loaded Package {package.Name} {package.VersionName} from {package.RootDirectory}");
PackgeLoaded?.Invoke(package);
Expand Down
121 changes: 107 additions & 14 deletions src/NodeServices/PythonServices.cs
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,52 @@
public static PythonEngineManager Instance { get { return lazy.Value; } }
#endregion

//TODO see DYN-6550 when hiding/replacing this obsolete field.
/// <summary>
/// A readonly collection of all the loaded Python engines
/// </summary>
public ReadOnlyCollection<PythonEngine> PythonEngines => new ReadOnlyCollection<PythonEngine>(AvailableEngines);

/// <summary>
/// An observable collection of all the loaded Python engines
/// </summary>
[Obsolete("AvailableEngines field will be replaced in a future Dynamo release.")]
public ObservableCollection<PythonEngine> AvailableEngines;
internal ObservableCollection<PythonEngine> AvailableEngines;

public delegate void PythonEngineChangedHandler(PythonEngine engine);

private Action<PythonEngine> customizeEngine;

/// <summary>
/// Use this function to customize Python engine initialization.
/// This function will be called only once on all existing or future python engines (on PythonEngineAdded).
Copy link
Member

Choose a reason for hiding this comment

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

comment is a bit misleading as this setter has the side effect of calling the method...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I kind of don't like a setter doing more than just replacing a value,
Still not convinced this is the proper way to go...

/// </summary>
public Action<PythonEngine> CustomizeEngine
Copy link
Member

Choose a reason for hiding this comment

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

this seems kind of dangerous, I imagine you want integrations like Revit to use this to setup marshallers and hook up events?

But this would also be easily accessible to anyone since this class has a static instance method - it could easily break python integration.

{
set
{
customizeEngine = value;
if (customizeEngine != null)
{
try
{
foreach (var engine in AvailableEngines)
{
customizeEngine(engine);
}
}
catch { }
}
}
}

/// <summary>
/// Event that is triggered for every new engine that is added.
/// </summary>
public static PythonEngineChangedHandler PythonEngineAdded;

/// <summary>
/// Event that is triggered for every engine that is removed.
/// </summary>
public static PythonEngineChangedHandler PythonEngineRemoved;
Copy link
Member

Choose a reason for hiding this comment

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

how do you imagine removal will work? Just remove the instance from the list of engines or actually try to unload it?

Copy link
Contributor Author

@pinzart90 pinzart90 Feb 14, 2024

Choose a reason for hiding this comment

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

With ALCs, I would imagine completely unloading it (assuming it works for python runtime)


#region Constant strings

Expand All @@ -132,16 +172,11 @@

internal static string PythonEvaluatorSingletonInstance = "Instance";

internal static string IronPythonEvaluatorClass = "IronPythonEvaluator";
internal static string IronPythonEvaluationMethod = "EvaluateIronPythonScript";

internal static string CPythonEvaluatorClass = "CPythonEvaluator";
internal static string CPythonEvaluationMethod = "EvaluatePythonScript";

internal static string IronPythonAssemblyName = "DSIronPython";
internal static string CPythonAssemblyName = "DSCPython";

internal static string IronPythonTypeName = IronPythonAssemblyName + "." + IronPythonEvaluatorClass;
internal static string CPythonTypeName = CPythonAssemblyName + "." + CPythonEvaluatorClass;

internal static string PythonInputMarshalerProperty = "InputMarshaler";
Expand All @@ -157,7 +192,7 @@
private PythonEngineManager()
{
AvailableEngines = new ObservableCollection<PythonEngine>();

AvailableEngines.CollectionChanged += AvailableEngines_CollectionChanged;
// We check only for the default python engine because it is the only one loaded by static references.
// Other engines can only be loaded through package manager
LoadDefaultPythonEngine(AppDomain.CurrentDomain.GetAssemblies().
Expand All @@ -166,6 +201,48 @@
AppDomain.CurrentDomain.AssemblyLoad += new AssemblyLoadEventHandler((object sender, AssemblyLoadEventArgs args) => LoadDefaultPythonEngine(args.LoadedAssembly));
}

private void AvailableEngines_CollectionChanged(object sender, System.Collections.Specialized.NotifyCollectionChangedEventArgs e)
{
if (e.Action == System.Collections.Specialized.NotifyCollectionChangedAction.Add)
{
for (var ii=0; ii< e.NewItems.Count; ii++)
{
try
{
PythonEngineAdded?.Invoke(e.NewItems[ii] as PythonEngine);
}
catch(Exception ex)
{
Console.WriteLine("PythonEngineAdded event failed with error : " + ex.Message + Environment.NewLine + ex.StackTrace);
}

try
{
customizeEngine.Invoke(e.NewItems[ii] as PythonEngine);
}
catch (Exception ex)
{
Console.WriteLine("CustomizeEngine failed with error : " + ex.Message + Environment.NewLine + ex.StackTrace);
}
}
}

if (e.Action == System.Collections.Specialized.NotifyCollectionChangedAction.Remove)
{
for (var ii = 0; ii < e.OldItems.Count; ii++)
{
try
{
PythonEngineRemoved?.Invoke(e.OldItems[ii] as PythonEngine);
}
catch (Exception ex)
{
Console.WriteLine("PythonEngineRemoved event failed with error : " + ex.Message + Environment.NewLine + ex.StackTrace);
}
}
}
}

private void LoadDefaultPythonEngine(Assembly a)
{
if (a == null ||
Expand All @@ -180,7 +257,7 @@
}
catch (Exception e)
{
System.Diagnostics.Debug.WriteLine($"Failed to load {CPythonAssemblyName} with error: {e.Message}");
Console.WriteLine($"Failed to load {CPythonAssemblyName} with error: {e.Message}");
}
}

Expand All @@ -189,16 +266,32 @@
return AvailableEngines.FirstOrDefault(x => x.Name == name);
}

// This method can throw exceptions.
internal void LoadPythonEngine(IEnumerable<Assembly> assemblies)
/// <summary>
/// Load Python Engines from an array of assemblies
/// </summary>
/// <param name="assemblies"></param>
internal void LoadPythonEngine(Action<string> logger, IEnumerable<Assembly> assemblies)

Check warning on line 273 in src/NodeServices/PythonServices.cs

View workflow job for this annotation

GitHub Actions / analyze

Parameter 'logger' has no matching param tag in the XML comment for 'PythonEngineManager.LoadPythonEngine(Action<string>, IEnumerable<Assembly>)' (but other parameters do)
{
foreach (var a in assemblies)
{
LoadPythonEngine(a);
try
{
LoadPythonEngine(a);
}
catch(Exception e)
{
logger?.Invoke("Failed when looking for PythonEngines with error: " + e.Message + Environment.NewLine + e.StackTrace);
}
}
}

// This method can throw exceptions.
/// <summary>
/// Load Python Engines from an assembly
/// </summary>
/// <param name="assembly"></param>
/// <exception cref="Exception"></exception>
/// Make this public to support custom loading of Python Engines (ex load in isolated alc on the python assembly side)
/// How to transition to Dynamo loading in isolated ALC ?
private void LoadPythonEngine(Assembly assembly)
{
if (assembly == null)
Expand Down
Loading