-
Notifications
You must be signed in to change notification settings - Fork 634
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
DYN-7535 Record python engine package information in graphs when using engines served from them #15515
DYN-7535 Record python engine package information in graphs when using engines served from them #15515
Changes from all commits
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 |
---|---|---|
|
@@ -6,6 +6,8 @@ | |
using System.Drawing; | ||
using System.IO; | ||
using System.Linq; | ||
using System.Reflection; | ||
using System.Runtime.InteropServices; | ||
using System.Windows; | ||
using Dynamo.Configuration; | ||
using Dynamo.Core; | ||
|
@@ -1314,6 +1316,78 @@ private void AddPythonEnginesOptions() | |
options.Add(item.Name); | ||
} | ||
PythonEnginesList = options; | ||
PopulatePythonEnginePackageMap(); | ||
} | ||
|
||
private void PopulatePythonEnginePackageMap() | ||
{ | ||
var packageBinPaths = new List<string>(); | ||
foreach (var packagePath in preferenceSettings.CustomPackageFolders) | ||
{ | ||
var ppath = packagePath == DynamoModel.BuiltInPackagesToken ? PathManager.BuiltinPackagesDirectory : packagePath; | ||
packageBinPaths = packageBinPaths.Concat(Directory.GetFiles(ppath, "*.dll", SearchOption.AllDirectories)).ToList(); | ||
} | ||
|
||
var dotNetRuntimePaths = Directory.GetFiles(RuntimeEnvironment.GetRuntimeDirectory(), "*.dll"); | ||
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. executing these calls over and over is a waste - disk access can be slow. |
||
var dynCorePaths = new FileInfo(Assembly.GetExecutingAssembly().Location).Directory.GetFiles("*.dll", SearchOption.AllDirectories).Select(x => x.FullName); | ||
var standardPaths = dotNetRuntimePaths.Concat(dynCorePaths); | ||
|
||
MetadataLoadContext mlc = null; | ||
var resolver = new PathAssemblyResolver(standardPaths); | ||
mlc = new MetadataLoadContext(resolver); | ||
var all = AppDomain.CurrentDomain.GetAssemblies().ToList(); | ||
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 worry about this because you'll only find assemblies loaded into the default ALC, I don't think this will let you see all the assemblies loaded into the current process. You may need to dig around in the dotnet source to confirm or deny that.
|
||
|
||
foreach (var packageBin in packageBinPaths) | ||
{ | ||
try | ||
{ | ||
//Load assembly into MLC | ||
var mlcassem = mlc.LoadFromAssemblyPath(packageBin); | ||
if (mlcassem != null) | ||
{ | ||
//Check if it consists of a Python Engine | ||
var a = mlcassem.GetTypes().FirstOrDefault(x => IsEqualPythonEngineAssemblyName(x) && !x.IsInterface && !x.IsAbstract); | ||
if (a != null) | ||
{ | ||
//Fetch it's loaded instance from AppDomain | ||
var b = all.FirstOrDefault(x => x.GetName().Name.ToLower().Equals(Path.GetFileNameWithoutExtension(mlcassem.GetName().Name).ToLower()), null); | ||
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. this could be very slow. |
||
if (b != null) | ||
{ | ||
//Validate result and add to map | ||
PythonEngineManager.Instance.ValidatePythonEngine(b, out var eng); | ||
if (PythonEnginesList.Contains(eng.Name)) | ||
{ | ||
var packageLoader = dynamoViewModel.Model.GetPackageManagerExtension()?.PackageLoader; | ||
var pkg = packageLoader.GetOwnerPackage(packageBin); | ||
var tup = new Tuple<string, string>(pkg.Name, pkg.VersionName); | ||
PythonEngineManager.Instance.PythonEnginePackageMap.Add(eng.Name, tup); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
catch | ||
{ | ||
// Ignore exceptions from iterating assembly types. | ||
continue; | ||
} | ||
} | ||
mlc?.Dispose(); | ||
} | ||
|
||
//ignoring assembly version | ||
private bool IsEqualPythonEngineAssemblyName(Type t) | ||
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. you found that this failed if the base types were of different versions? You cannot just check that the type is assignable to a 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 assembly returned from MLC is different from the assembly loaded into the App, it had partial information, and the isAssignable check to |
||
{ | ||
var pArr = typeof(PythonEngine).AssemblyQualifiedName?.Split(','); | ||
var tArr = t.BaseType?.AssemblyQualifiedName?.Split(','); | ||
if (pArr.Count() > 0 && tArr.Count() > 0) | ||
{ | ||
if (pArr[0].Equals(tArr[0]) && pArr[1].Equals(tArr[1])) | ||
{ | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
#endregion | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,8 @@ public PythonCodeMigrationEventArgs(string oldCode, string newCode) | |
public abstract class PythonNodeBase : VariableInputNode | ||
{ | ||
private string engine = string.Empty; | ||
private string enginePackageName = string.Empty; | ||
private string enginePackageVersion = string.Empty; | ||
|
||
// Set the default EngineName value to IronPython2 so that older graphs can show the migration warnings. | ||
[DefaultValue("IronPython2")] | ||
|
@@ -58,11 +60,50 @@ public string EngineName | |
if (engine != value) | ||
{ | ||
engine = value; | ||
SetEnginePackageInfo(engine); | ||
RaisePropertyChanged(nameof(EngineName)); | ||
} | ||
} | ||
} | ||
|
||
[JsonProperty("EnginePackageName")] | ||
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. Is there value in serializing this with each node? 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 graph containing multiple python nodes, pointing to different engines, imported from different packages. |
||
/// <summary> | ||
/// Return the package name of the selected python engine, if it was loaded from an extenal package. | ||
/// </summary> | ||
public string EnginePackageName | ||
{ | ||
get | ||
{ | ||
return enginePackageName; | ||
} | ||
set | ||
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. definitely weird that this is a public setter. |
||
{ | ||
if (enginePackageName != value) | ||
{ | ||
enginePackageName = value; | ||
} | ||
} | ||
} | ||
|
||
[JsonProperty("EnginePackageVersion")] | ||
/// <summary> | ||
/// Return the package version of the selected python engine, if it was loaded from an extenal package. | ||
/// </summary> | ||
public string EnginePackageVersion | ||
{ | ||
get | ||
{ | ||
return enginePackageVersion; | ||
} | ||
set | ||
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 here. |
||
{ | ||
if (enginePackageVersion != value) | ||
{ | ||
enginePackageVersion = value; | ||
} | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Set the engine to be used by default for this node, based on user and system settings. | ||
/// </summary> | ||
|
@@ -83,6 +124,16 @@ private void SetEngineByDefault() | |
// Use CPython as default | ||
engine = PythonEngineManager.CPython3EngineName; | ||
} | ||
SetEnginePackageInfo(engine); | ||
} | ||
private void SetEnginePackageInfo(string engine) | ||
{ | ||
var pkgDetails = PythonEngineManager.TryGetPythonEnginePackage(engine); | ||
if (pkgDetails != null) | ||
{ | ||
enginePackageName = pkgDetails.Item1; | ||
enginePackageVersion = pkgDetails.Item2; | ||
} | ||
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 would expect that all engine names stored throughout Dynamo should be changed to something like FullyQualifiedEngineName (PackageName.EngineName@PkgVersion) So we should not need to match the EngineName to packages. |
||
} | ||
|
||
protected PythonNodeBase() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,8 +108,7 @@ internal static readonly Lazy<PythonEngineManager> | |
new Lazy<PythonEngineManager> | ||
(() => new PythonEngineManager()); | ||
|
||
private readonly string[] dotNetRuntimePaths; | ||
private readonly IEnumerable<string> dynCorePaths; | ||
private readonly IEnumerable<string> standardPaths; | ||
|
||
#region Public members | ||
/// <summary> | ||
|
@@ -125,8 +124,10 @@ internal static readonly Lazy<PythonEngineManager> | |
[Obsolete("AvailableEngines field will be replaced in a future Dynamo release.")] | ||
public ObservableCollection<PythonEngine> AvailableEngines; | ||
|
||
internal Dictionary<string, Tuple<string, string>> PythonEnginePackageMap; | ||
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. why not add the Package and Version properties to the PythonEngine class ? 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. Yes, but it is not straight-forward, the python engine is loaded as an extension, and extensions are loaded after packages, so we do not know if an engine was added after the packages are loaded. |
||
|
||
#region Constant strings | ||
|
||
/// <summary> | ||
/// CPython Engine name | ||
/// </summary> | ||
|
@@ -162,10 +163,12 @@ internal static readonly Lazy<PythonEngineManager> | |
/// </summary> | ||
private PythonEngineManager() | ||
{ | ||
dotNetRuntimePaths = Directory.GetFiles(RuntimeEnvironment.GetRuntimeDirectory(), "*.dll"); | ||
dynCorePaths = new FileInfo(Assembly.GetExecutingAssembly().Location).Directory.GetFiles("*.dll", SearchOption.AllDirectories).Select(x => x.FullName); | ||
var dotNetRuntimePaths = Directory.GetFiles(RuntimeEnvironment.GetRuntimeDirectory(), "*.dll"); | ||
var dynCorePaths = new FileInfo(Assembly.GetExecutingAssembly().Location).Directory.GetFiles("*.dll", SearchOption.AllDirectories).Select(x => x.FullName); | ||
standardPaths = dotNetRuntimePaths.Concat(dynCorePaths); | ||
|
||
AvailableEngines = new ObservableCollection<PythonEngine>(); | ||
PythonEnginePackageMap = new Dictionary<string, Tuple<string, string>>(); | ||
|
||
// 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 | ||
|
@@ -175,6 +178,18 @@ private PythonEngineManager() | |
AppDomain.CurrentDomain.AssemblyLoad += new AssemblyLoadEventHandler((object sender, AssemblyLoadEventArgs args) => LoadDefaultPythonEngine(args.LoadedAssembly)); | ||
} | ||
|
||
internal static Tuple<string, string> TryGetPythonEnginePackage(string engineName) | ||
{ | ||
if (Instance.PythonEnginePackageMap.Count > 0) | ||
{ | ||
if (Instance.PythonEnginePackageMap.TryGetValue(engineName, out var tuple)) | ||
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. what happens when there are engines with the same 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. will add a check |
||
{ | ||
return tuple; | ||
} | ||
} | ||
return null; | ||
} | ||
|
||
private void LoadDefaultPythonEngine(Assembly a) | ||
{ | ||
if (a == null || | ||
|
@@ -210,9 +225,30 @@ internal void LoadPythonEngine(IEnumerable<Assembly> assemblies) | |
// This method can throw exceptions. | ||
private void LoadPythonEngine(Assembly assembly) | ||
{ | ||
try | ||
{ | ||
if (!ValidatePythonEngine(assembly, out var engine)) return; | ||
|
||
VerifyEngineReferences(assembly, standardPaths); | ||
|
||
if (GetEngine(engine.Name) == null) | ||
{ | ||
AvailableEngines.Add(engine); | ||
} | ||
} | ||
catch (Exception ex) | ||
{ | ||
throw new Exception($"Failed to add a Python engine from assembly {assembly.GetName().Name}.dll with error: {ex.Message}"); | ||
} | ||
} | ||
|
||
// This method can throw exceptions. | ||
internal bool ValidatePythonEngine(Assembly assembly, out PythonEngine eng) | ||
{ | ||
eng = null; | ||
if (assembly == null) | ||
{ | ||
return; | ||
return false; | ||
} | ||
// Currently we are using try-catch to validate loaded assembly and Singleton Instance method exist | ||
// but we can optimize by checking all loaded types against evaluators interface later | ||
|
@@ -222,36 +258,35 @@ private void LoadPythonEngine(Assembly assembly) | |
PropertyInfo instanceProp = null; | ||
try | ||
{ | ||
//eType = assembly.GetTypes().FirstOrDefault(x => typeof(PythonEngine).IsAssignableFrom(x) && !x.IsInterface && !x.IsAbstract); | ||
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. ?? |
||
|
||
eType = assembly.GetTypes().FirstOrDefault(x => typeof(PythonEngine).IsAssignableFrom(x) && !x.IsInterface && !x.IsAbstract); | ||
if (eType == null) return; | ||
if (eType == null) return false; | ||
|
||
instanceProp = eType?.GetProperty(PythonEvaluatorSingletonInstance, BindingFlags.NonPublic | BindingFlags.Static); | ||
if (instanceProp == null) return; | ||
if (instanceProp == null) return false; | ||
} | ||
catch | ||
{ | ||
// Ignore exceptions from iterating assembly types. | ||
return; | ||
return false; | ||
} | ||
|
||
PythonEngine engine = (PythonEngine)instanceProp.GetValue(null); | ||
if (engine == null) | ||
{ | ||
throw new Exception($"Could not get a valid PythonEngine instance by calling the {eType.Name}.{PythonEvaluatorSingletonInstance} method"); | ||
} | ||
|
||
VerifyEngineReferences(assembly,dotNetRuntimePaths.Concat(dynCorePaths)); | ||
|
||
if (GetEngine(engine.Name) == null) | ||
{ | ||
AvailableEngines.Add(engine); | ||
} | ||
eng = engine; | ||
return true; | ||
} | ||
catch (Exception ex) | ||
{ | ||
throw new Exception($"Failed to add a Python engine from assembly {assembly.GetName().Name}.dll with error: {ex.Message}"); | ||
throw new Exception($"Failed to validate the Python engine from assembly {assembly.GetName().Name}.dll with error: {ex.Message}"); | ||
} | ||
} | ||
|
||
|
||
/// <summary> | ||
/// Attempts to verify that the dependencies of the given assembly can be found and loaded into an MetadataLoadContext. | ||
/// Will throw exceptions if assemblies cannot be loaded. | ||
|
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.
why are you doing all this assembly searching and loading here ?
PythonEngines are already loaded In the PythonEngineManager constructor and in the PackageManager.PackageLOader.
The
PythonEnginesList
from this class will always be kept up to date whenPythonEngineManager.Instance.AvailableEngines
is changed. We should be able to store all the info we need in the PythonENgine classes when they are initialized.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.
Had to do all this to avoid breaking changes, also the idea was to not depend on the extension/engine to tell what package they come from (at-least for me), if allowed to change PythonEngine, or add a dependency on the extension itself to declare which package they belong to, it would have been way simpler. I agree.