use ALC for deps #1
Conversation
There was a problem hiding this comment.
@mjkkirschner are you saying that you want to move this logic to the IronPython3 package so that this IronPython2 package can load in the default assembly context while the former loads isolated in a new ALC?
Thats what I was thinking - my logic was that IronPython2 is used widely and IronPython3 is not. Using the default load context is closer to what was occurring in Dynamo 2.x so theres less chance of unanticipated changes - BUT because we can release these updates to the packages at any time I feel it's not critical. Alternatively, as an experiment it might be nice to isolate both packages and see what happens with actual users, then if we find problems we can release new versions. This could be a good first test of isolating ALL packages. What do you think? |
| } | ||
| var extraPath = Path.Combine(new FileInfo(Assembly.GetAssembly(typeof(IronPythonExtension)).Location).Directory.Parent.FullName, "extra"); | ||
| var alc = new IsolatedPythoContext(Path.Combine(extraPath,"DSIronPython.dll")); | ||
| alc.LoadFromAssemblyName(new AssemblyName("DSIronPython")); |
There was a problem hiding this comment.
this means that only the DSIronPython.dll assembly (and its own dependencies) will be isolated
The IronPythonExtension assembly will be part of Dynamo's ALC
This sounds fine to me. I did the same thing with Dynamo4Revit and it worked ok
As long as the IronPythonExtension 's dependencies are ok to be in Dynamo's ALC
There was a problem hiding this comment.
Also I do not remember how the dependencies of the entrypoint (DSIronPython.dll) are resolved.
Hopefully it happens magically by using the ALC resolver and the deps.json file
I remember there were some issues with the dependencies that required special Resolver code in the Host app (that would be Dynamo in this case)
There was a problem hiding this comment.
Please double check how the dependencies of DSIronPython.dll are resolved and in which ALC
There was a problem hiding this comment.
yes, correct IronPythonExtension basically has no dependencies except for Dynamo and at least my testing indicates that the assemblies are isolated from the default ALC (since ironPython3 (not isolated) works side by side))
I think I can double check by using The GetAssemblyLoadContext() method on each assembly, or maybe using dotnet-trace tooling see exactly when the load occurs.
Any other ideas for how to test Please double check how the dependencies of DSIronPython.dll are resolved and in which ALC besides the ideas above?
There was a problem hiding this comment.
Harder to do, but you can simply put breakpoints in all the resolvers in Dynamo and in this extension in the Assembly Load(AssemblyName assemblyName) and implement https://learn.microsoft.com/en-us/dotnet/api/system.runtime.loader.assemblyloadcontext.resolving?view=net-8.0
You could implement that in the dynamo default ALC and put a breakpoint too
There was a problem hiding this comment.
Here is the algorithm, we use the name variant:
https://learn.microsoft.com/en-us/dotnet/core/dependency-loading/loading-managed#algorithm
I'm pretty sure that dependencies of the assembly that are loaded using by name - are loaded into the same ALC, or at least as indicated by the algorithm, an attempt is made to resolve them before falling back to the default ALC.
Image shows the dependencies loaded side by side
image shows the assemblies loaded into the python2.xALC
I checked the above with the deps.json files in the DSIronPython folder, and without them and got the same functioning behavior of side by side iron python versions and the same binaries loaded into the ALC.
There was a problem hiding this comment.
This was the issue I was thinking of dotnet/runtime#93780 (comment)
But this does not seem to be the case with IronPython deps. They probably are all in the resolver folder
I will likely move these change to the DSIronPython3 package instead, but adding this for feedback since the changes will look exactly the same there.
This requires one change on the Dynamo side that I need to check with @pinzart90about.personally I prefer the solution in this repo because it means we don't risk making any changes to Dynamo before release, and we instead can update at any time by updating the python engine packages.
The two important parts of this PR are: