Skip to content

Modernize import hook #13

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

Draft
wants to merge 163 commits into
base: upstream
Choose a base branch
from
Draft

Modernize import hook #13

wants to merge 163 commits into from

Conversation

BadSingleton
Copy link

What does this implement/fix? Explain your changes.

This replaces the current import hook replacement with a MetaPathFinder, letting python handle non-CLR module import.

Does this close any currently open issues?

pythonnet#727 pythonnet#1245 pythonnet#547 pythonnet#111 and probably others.

Any other comments?

This fix also helps working around a mono runtime behaviour where a mono owns all threads that execute .NET code.

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

lostmsu added 14 commits May 13, 2020 14:03
…ET interop

New method ThrowLastAsClrException should be used everywhere instead of obsolete PythonException constructor. It automatically restores .NET exceptions, and applies codecs for Python exceptions.

Traceback, PyVal and PyType are now stored and returned as PyObjects.

PythonException now has InnerException set from its cause (e.g. __cause__ in Python, if any).

PythonException.Restore no longer clears the exception instance.

All helper methods were removed from public API surface.
The new type indicates parameters of C API functions,
that steal references to passed objects.
@@ -47,6 +48,7 @@ details about the cause of the failure
- Incorrectly using a non-generic type with type parameters now produces a helpful Python error instead of throwing NullReferenceException
- `import` may now raise errors with more detail than "No module named X"


Choose a reason for hiding this comment

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

remove the extra whitespace

},
new TestCase
{
// The C# code for this test doesn't matter.

Choose a reason for hiding this comment

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

Please expand a little more: We're testing that the import hook in Python reloads properly, so the C# code for this test doesn't matter.

(in which case why have have any code at all? Can you compile an empty string?)

// For some reason, exceptions happening here does... nothing.
// Even System.AccessViolationExceptions gets ignored.
Console.WriteLine($"Namespace added callback failed with: {e}");
throw;

Choose a reason for hiding this comment

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

Why catch and rethrow? Just to get a log?

Copy link
Author

@BadSingleton BadSingleton Jan 20, 2021

Choose a reason for hiding this comment

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

The throw does nothing; the caller catches all exceptions. It might be a bug in the runtime (it might throw appropriately in mono), it might change in the future, so I'm rethrowing. I'm also logging because it took a long time to find why thing were borken further down the execution.

class DotNetFinder(importlib.abc.MetaPathFinder):

def __init__(self):
print('DotNetFinder init')

Choose a reason for hiding this comment

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

probably remove this

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that should have been removed

/// Return the clr python module (new reference)
/// </summary>
public static IntPtr GetCLRModule(IntPtr? fromList = null)
static void SetupNamespaceTracking ()

Choose a reason for hiding this comment

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

A header comment here would be nice, answering: what is namespace tracking?

Copy link

@benoithudson benoithudson left a comment

Choose a reason for hiding this comment

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

Nothing substantial on the code; I think it would be nice to have some comments though in any new function you created.

@BadSingleton BadSingleton force-pushed the modernize-import-hook branch 2 times, most recently from 200ba3d to f502b8e Compare January 25, 2021 19:50
tminka and others added 3 commits January 27, 2021 12:12
…net#1361)

Better error messages for method argument mismatch, Python to managed value conversion, and other problems.

- Converter.ToManaged obeys setError consistently
- PyObject_Hash and tp_hash return nint
- MakeGenericType and MakeGenericMethod have try/catch
- RaiseTypeError sets __cause__ instead of changing the message string
- PythonException.Normalize throws if an error is set
- AggregateExceptions print better from Python
- For an overloaded method, MethodBinder.Bind collects all binding errors with PythonExceptions as the inner.inner exceptions.
- ExceptionClassObject.tp_str calls Exception.ToString() instead of Exception.Message
(for justification see https://stackoverflow.com/questions/2176707/exception-message-vs-exception-tostring )
- Binding failures always have AggregateException as the cause, 
- Disabled test_method_parameters_change
…ers in Python, by returning the modified parameter values in a tuple.

BREAKING: MethodBinder omits a void return type when returning a tuple of out parameters.
DelegateManager unpacks a tuple of out parameters from Python (reversing the logic in MethodBinder) and sets the out parameters of the delegate.
@BadSingleton BadSingleton force-pushed the modernize-import-hook branch from 2860632 to 970a189 Compare June 2, 2021 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants