-
Notifications
You must be signed in to change notification settings - Fork 0
add sample python connector with pythonnet #322
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #322 +/- ##
==========================================
+ Coverage 74.59% 75.48% +0.88%
==========================================
Files 38 38
Lines 4074 4083 +9
Branches 487 487
==========================================
+ Hits 3039 3082 +43
+ Misses 852 817 -35
- Partials 183 184 +1 🚀 New features to boost your workflow:
|
|
/gemini review |
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.
Code Review
This pull request introduces a new example connector that demonstrates how to integrate a Python-based simulator using pythonnet. The overall architecture is well-designed, with a clear separation between the C# bridge and the Python implementation. The code is generally of high quality. I've identified a few areas for improvement related to robustness and asynchronous programming best practices. Specifically, I've suggested changes to avoid blocking on async calls in the main entry point and to handle missing configuration from Python scripts more gracefully to prevent runtime crashes. These changes will make the example more robust and a better foundation for users to build upon.
|
/gemini review |
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.
Code Review
This pull request introduces a new Python connector for MuJoCo using pythonnet. The architecture is well-designed, embedding Python scripts and bridging them from C#. The review identified a few significant issues: a critical security vulnerability in path validation, a high-severity bug that loses exception stack traces during Python interop, and a high-severity issue in the main entry point related to asynchronous programming and error handling. These should be addressed to ensure the connector is secure, robust, and debuggable.
| var normalizedInput = Path.GetFullPath(path); | ||
| if (normalizedInput != fullPath) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Python path normalization mismatch: '{path}' -> '{fullPath}'. " + | ||
| "This may indicate a path traversal attempt."); | ||
| } |
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.
This path traversal security check is non-functional. normalizedInput is calculated on line 69 using Path.GetFullPath(path), which is the same way fullPath was calculated on line 53. As a result, normalizedInput will always be equal to fullPath, and this check will never fail. This leaves a security vulnerability where a path containing .. sequences could be used to access directories outside of the intended scope, leading to potential loading of malicious Python modules.
| public static int Main(string[] args) | ||
| { | ||
| try | ||
| { | ||
| ConnectorRuntime.RunStandalone().Wait(); | ||
| return 0; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Console.Error.WriteLine($"Fatal error: {ex.Message}"); | ||
| if (ex.InnerException != null) | ||
| { | ||
| Console.Error.WriteLine($"Inner exception: {ex.InnerException.Message}"); | ||
| } | ||
| return 1; | ||
| } | ||
| } |
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.
The Main method blocks on an async task using .Wait(), which is a discouraged practice that can lead to deadlocks. Additionally, the top-level exception handler only logs the exception message, omitting the stack trace and other crucial details, which severely hampers debugging of fatal errors.
public static async Task<int> Main(string[] args)
{
try
{
await ConnectorRuntime.RunStandalone().ConfigureAwait(false);
return 0;
}
catch (Exception ex)
{
Console.Error.WriteLine($"Fatal error: {ex}");
return 1;
}
}| protected T RunPythonWithLargeStack<T>(Func<T> pythonCall, string operationName, bool isSimulation = false) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(pythonCall); | ||
| ArgumentNullException.ThrowIfNull(operationName); | ||
|
|
||
| T result = default!; | ||
| Exception? caughtException = null; | ||
|
|
||
| const int stackSize = 8 * 1024 * 1024; | ||
|
|
||
| var thread = new Thread(() => | ||
| { | ||
| try | ||
| { | ||
| result = RunPython(pythonCall, operationName, isSimulation); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| caughtException = ex; | ||
| } | ||
| }, stackSize); | ||
|
|
||
| thread.Start(); | ||
| thread.Join(); | ||
|
|
||
| if (caughtException != null) | ||
| { | ||
| throw caughtException; | ||
| } | ||
|
|
||
| return result; | ||
| } |
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.
The RunPythonWithLargeStack method catches an exception on a worker thread and re-throws it on the calling thread using throw caughtException;. This action resets the exception's stack trace, making it very difficult to debug the original source of the error.
protected T RunPythonWithLargeStack<T>(Func<T> pythonCall, string operationName, bool isSimulation = false)
{
ArgumentNullException.ThrowIfNull(pythonCall);
ArgumentNullException.ThrowIfNull(operationName);
T result = default!;
System.Runtime.ExceptionServices.ExceptionDispatchInfo? edi = null;
const int stackSize = 8 * 1024 * 1024;
var thread = new Thread(() =>
{
try
{
result = RunPython(pythonCall, operationName, isSimulation);
}
catch (Exception ex)
{
edi = System.Runtime.ExceptionServices.ExceptionDispatchInfo.Capture(ex);
}
}, stackSize);
thread.Start();
thread.Join();
edi?.Throw();
return result;
}
Add Python connector sample that lets users write simulator connectors entirely in Python using pythonnet to bridge to the .NET framework. Users implement three Python files (definition.py, client.py, routine.py) while C# bridge handles framework integration.
MuJoCo Python Connector
A sample connector using pythonnet to integrate MuJoCo physics simulator with CDF. Python files are embedded in the executable at build time.
Architecture
Prerequisites
pip install mujoco find-libpythonAbout: