-
Notifications
You must be signed in to change notification settings - Fork 139
feat: First tentative for Plugin Mapdl Mechanism python API #3627
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
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
I will check it out next year. #HappyVacations |
Can't wait to discuss about it early next year when I visit you @FredAns 😃 . |
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.
I think this is good. I would also add a string dunder method __str__
, so we can do:
>>> print(mapdl.plugins)
MAPDL Plugins
----------------
DPF : feature
ABC : feature
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3627 +/- ##
==========================================
- Coverage 91.28% 87.40% -3.88%
==========================================
Files 193 194 +1
Lines 15719 15811 +92
==========================================
- Hits 14349 13820 -529
- Misses 1370 1991 +621 🚀 New features to boost your workflow:
|
From @FredAns:
|
Nevermind |
@FredAns can we rely on the
|
…mand parsing and lifecycle management
Reviewer's GuideAdds a new plugin mechanism via a dedicated ansPlugin manager integrated into the Mapdl class, enabling dynamic loading, unloading, and listing of external MAPDL plugins along with automatic command injection and removal. Sequence diagram for loading a plugin via ansPluginsequenceDiagram
actor User
participant Mapdl
participant ansPlugin
participant MAPDL_Server
User->>Mapdl: mapdl.plugins.load('PluginDPF')
Mapdl->>ansPlugin: plugins property (init if needed)
ansPlugin->>MAPDL_Server: *PLUG,LOAD,PluginDPF
MAPDL_Server-->>ansPlugin: Response (with registered commands)
ansPlugin->>ansPlugin: _parse_commands(response)
ansPlugin->>Mapdl: Inject new commands as methods
ansPlugin-->>User: Return response
Sequence diagram for unloading a plugin via ansPluginsequenceDiagram
actor User
participant Mapdl
participant ansPlugin
participant MAPDL_Server
User->>Mapdl: mapdl.plugins.unload('PluginDPF')
Mapdl->>ansPlugin: plugins property
ansPlugin->>MAPDL_Server: *PLUG,UNLOAD,PluginDPF
MAPDL_Server-->>ansPlugin: Response (with commands to remove)
ansPlugin->>ansPlugin: _parse_commands(response)
ansPlugin->>Mapdl: Remove injected commands
ansPlugin-->>User: Return response
Class diagram for the new ansPlugin manager and related error classesclassDiagram
class Mapdl {
+ansPlugin plugins
}
class ansPlugin {
-weakref _mapdl_weakref
-str _filename
-bool _open
+load(plugin_name, feature="") str
+unload(plugin_name) str
+list() list[str]
-_parse_commands(response) list[str]
-_set_commands(commands, plugin_name)
-_deleter_commands(commands, plugin_name)
-_load_commands(response, plugin_name)
+_mapdl: Mapdl
+_log: Logger
}
class PluginError {
}
class PluginLoadError {
}
class PluginUnloadError {
}
class MapdlRuntimeError {
}
class Logger {
}
Mapdl --> ansPlugin : uses
ansPlugin --> Mapdl : weakref
ansPlugin --> Logger : uses
PluginError --|> MapdlRuntimeError
PluginLoadError --|> PluginError
PluginUnloadError --|> PluginError
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @FredAns - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/ansys/mapdl/core/plugin.py:102` </location>
<code_context>
+ mapdl = self._mapdl
+
+ for each_command in commands:
+ each_command.replace("*", "star")
+ each_command.replace("/", "slash")
+
+ if hasattr(mapdl, each_command):
</code_context>
<issue_to_address>
String replacements on 'each_command' are not assigned back, so they have no effect.
Since strings are immutable in Python, assign the result of 'replace' back to 'each_command' to ensure the changes take effect.
</issue_to_address>
### Comment 2
<location> `src/ansys/mapdl/core/plugin.py:109` </location>
<code_context>
+ # We are allowing to overwrite existing commands
+ warn(f"Command '{each_command}' already exists in the MAPDL instance.")
+
+ def passer(self, *args, **kwargs):
+ return self.run(*args, **kwargs)
+
+ # Inject docstring
</code_context>
<issue_to_address>
The dynamically created 'passer' function may not bind correctly to the MAPDL instance.
Assigning 'passer' directly may cause binding issues with 'self'. Use functools.partial or types.MethodType to properly bind the method to the MAPDL instance.
</issue_to_address>
### Comment 3
<location> `src/ansys/mapdl/core/plugin.py:217` </location>
<code_context>
+ if "error" in response.lower():
+ raise PluginUnloadError(f"Failed to unload plugin '{plugin_name}'.")
+
+ self._load_commands(response, plugin_name)
+ self._log.info(f"Plugin '{plugin_name}' unloaded successfully.")
+
+ commands = self._parse_commands(response)
</code_context>
<issue_to_address>
Calling '_load_commands' during plugin unload may be unnecessary or confusing.
Reloading commands here may re-add those meant to be removed. Please review if this call is necessary or clarify its intent.
</issue_to_address>
### Comment 4
<location> `src/ansys/mapdl/core/plugin.py:246` </location>
<code_context>
+ raise PluginError("Failed to retrieve the list of loaded plugins.")
+
+ # Parse response and extract plugin names (assuming response is newline-separated text)
+ plugins = [line.strip() for line in response.splitlines() if line.strip()]
+ return plugins
</code_context>
<issue_to_address>
Plugin list parsing may include non-plugin lines if the response format changes.
The implementation treats all non-empty lines as plugin names, which could include unrelated text if the response format changes. Please add validation to ensure only actual plugin names are returned.
</issue_to_address>
### Comment 5
<location> `tests/test_plugin.py:64` </location>
<code_context>
+ assert TEST_PLUGIN in plugins.list(), "Plugin should be loaded"
+
+
+def test_plugin_unload(plugins):
+ plugins.unload(TEST_PLUGIN)
+ assert TEST_PLUGIN not in plugins.list(), "Plugin should be unloaded"
</code_context>
<issue_to_address>
Missing test for unloading a plugin that is not loaded.
Add a test to verify that unloading a plugin that is not loaded raises PluginUnloadError.
</issue_to_address>
### Comment 6
<location> `tests/test_plugin.py:69` </location>
<code_context>
+ assert TEST_PLUGIN not in plugins.list(), "Plugin should be unloaded"
+
+
+def test_parse_commands(plugins, dpf_load_response):
+ commands = plugins._parse_commands(dpf_load_response)
+
</code_context>
<issue_to_address>
No test for _parse_commands with empty or malformed response.
Add tests for _parse_commands to cover empty and malformed responses, verifying it returns an empty list without raising exceptions.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_parse_commands(plugins, dpf_load_response):
commands = plugins._parse_commands(dpf_load_response)
=======
def test_parse_commands(plugins, dpf_load_response):
commands = plugins._parse_commands(dpf_load_response)
# Existing test can have assertions if needed
def test_parse_commands_empty_response(plugins):
"""Test _parse_commands with an empty response."""
empty_response = ""
commands = plugins._parse_commands(empty_response)
assert commands == [], "Expected empty list for empty response"
def test_parse_commands_malformed_response(plugins):
"""Test _parse_commands with a malformed response."""
malformed_response = "This is not a valid command list"
commands = plugins._parse_commands(malformed_response)
assert commands == [], "Expected empty list for malformed response"
>>>>>>> REPLACE
</suggested_fix>
### Comment 7
<location> `tests/test_plugin.py:77` </location>
<code_context>
+ assert "*DPF" in commands, "Expected command '*DPF' should be in the list"
+
+
+def test_load_commands(plugins, dpf_load_response):
+ commands = plugins._parse_commands(dpf_load_response)
+ assert isinstance(commands, list), "Commands should be a list"
</code_context>
<issue_to_address>
No test for _set_commands overwriting existing commands.
Add a test to verify that calling _set_commands with an existing command name triggers the warning and overwrites the command as expected.
Suggested implementation:
```python
def test_load_commands(plugins, dpf_load_response):
commands = plugins._parse_commands(dpf_load_response)
assert isinstance(commands, list), "Commands should be a list"
assert len(commands) > 0, "Commands list should not be empty"
for command in commands:
def test_set_commands_overwrites_existing(monkeypatch, plugins):
"""
Test that _set_commands overwrites an existing command and triggers a warning.
"""
# Setup: Add a command
plugins._commands = {"EXISTING": lambda: "old"}
warnings = []
# Monkeypatch the warning mechanism if it's using warnings.warn or logger.warning
def fake_warn(msg, *args, **kwargs):
warnings.append(msg)
if hasattr(plugins, "logger"):
monkeypatch.setattr(plugins.logger, "warning", fake_warn)
else:
import warnings as pywarnings
monkeypatch.setattr(pywarnings, "warn", fake_warn)
# Overwrite the command
def new_func():
return "new"
plugins._set_commands({"EXISTING": new_func})
# Check that the warning was triggered
assert any("EXISTING" in str(w) for w in warnings), "Expected warning about overwriting command"
# Check that the command was overwritten
assert plugins._commands["EXISTING"] is new_func, "Command should be overwritten with new function"
```
- If your codebase uses a different warning or logging mechanism, adjust the monkeypatching accordingly.
- If `_set_commands` is not a public method, you may need to access it differently or adjust test visibility.
- If the warning is not triggered via `warnings.warn` or `logger.warning`, update the test to capture the correct warning mechanism.
</issue_to_address>
### Comment 8
<location> `tests/test_plugin.py:86` </location>
<code_context>
+ assert hasattr(plugins._mapdl, command)
+
+
+def test_deleter_commands(plugins, dpf_load_response):
+ commands = plugins._parse_commands(dpf_load_response)
+ assert isinstance(commands, list), "Commands should be a list"
</code_context>
<issue_to_address>
No test for _deleter_commands with commands that do not exist.
Please add a test to verify that _deleter_commands handles non-existent command names gracefully without raising errors.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
def passer(self, *args, **kwargs): | ||
return self.run(*args, **kwargs) |
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.
issue (bug_risk): The dynamically created 'passer' function may not bind correctly to the MAPDL instance.
Assigning 'passer' directly may cause binding issues with 'self'. Use functools.partial or types.MethodType to properly bind the method to the MAPDL instance.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…test for unloading plugin twice
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.
Pull Request Overview
This PR introduces a Python API for MAPDL's plugin mechanism, allowing users to dynamically load, unload, and manage external plugins (such as PluginDPF and gRPC server libraries) within a MAPDL session using the *PLUG
command.
Key changes:
- Implementation of
ansPlugin
class to manage plugin lifecycle operations - Integration of plugin manager into the core MAPDL interface via
mapdl.plugins
property - Addition of plugin-specific error classes for better error handling
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
src/ansys/mapdl/core/plugin.py | Core implementation of the plugin manager with load/unload/list operations and dynamic command injection |
src/ansys/mapdl/core/mapdl_core.py | Integration of plugin manager into MAPDL core class as a property |
src/ansys/mapdl/core/errors.py | New plugin-specific exception classes for error handling |
tests/test_plugin.py | Comprehensive test suite covering plugin operations and command management |
doc/changelog.d/3627.miscellaneous.md | Changelog entry documenting the new feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
from ansys.mapdl.core import Mapdl | ||
from ansys.mapdl.core.plugin import ansPlugin | ||
|
||
pytestmark = pytest.mark.random_order(disabled=True) |
Copilot
AI
Oct 14, 2025
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 pytestmark
variable is defined twice (lines 27 and 32), which is redundant. Remove the duplicate declaration on line 32.
pytestmark = pytest.mark.random_order(disabled=True) |
Copilot uses AI. Check for mistakes.
def passer(self, *args, **kwargs): | ||
return self.run(*args, **kwargs) | ||
|
||
# Inject docstring | ||
passer.__doc__ = f"""Command from plugin {plugin_name}: {each_command}. | ||
Use this plugin documentation to understand the command and its parameters. | ||
Automatically generated docstring by ansPlugin. | ||
""" |
Copilot
AI
Oct 14, 2025
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 passer
function is defined inside a loop and closes over the loop variable each_command
, but doesn't use it. This creates a closure issue where all injected commands share the same function object. The function should be created with proper binding to ensure each command gets its own callable.
def passer(self, *args, **kwargs): | |
return self.run(*args, **kwargs) | |
# Inject docstring | |
passer.__doc__ = f"""Command from plugin {plugin_name}: {each_command}. | |
Use this plugin documentation to understand the command and its parameters. | |
Automatically generated docstring by ansPlugin. | |
""" | |
def make_passer(docstring): | |
def passer(self, *args, **kwargs): | |
return self.run(*args, **kwargs) | |
passer.__doc__ = docstring | |
return passer | |
docstring = f"""Command from plugin {plugin_name}: {each_command}. | |
Use this plugin documentation to understand the command and its parameters. | |
Automatically generated docstring by ansPlugin. | |
""" | |
passer = make_passer(docstring) |
Copilot uses AI. Check for mistakes.
def passer(self, *args, **kwargs): | ||
return self.run(*args, **kwargs) | ||
|
||
# Inject docstring | ||
passer.__doc__ = f"""Command from plugin {plugin_name}: {each_command}. | ||
Use this plugin documentation to understand the command and its parameters. | ||
Automatically generated docstring by ansPlugin. | ||
""" | ||
setattr(mapdl, each_command, passer) |
Copilot
AI
Oct 14, 2025
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 passer
function is being reused for all commands in the loop. Since it's created once and reused, all commands will share the same method object, and the docstring will only reflect the last command processed. Create a new function instance for each command, possibly using a factory pattern or lambda with proper binding.
def passer(self, *args, **kwargs): | |
return self.run(*args, **kwargs) | |
# Inject docstring | |
passer.__doc__ = f"""Command from plugin {plugin_name}: {each_command}. | |
Use this plugin documentation to understand the command and its parameters. | |
Automatically generated docstring by ansPlugin. | |
""" | |
setattr(mapdl, each_command, passer) | |
def make_passer(plugin_name, command_name): | |
def passer(self, *args, **kwargs): | |
return self.run(*args, **kwargs) | |
passer.__doc__ = f"""Command from plugin {plugin_name}: {command_name}. | |
Use this plugin documentation to understand the command and its parameters. | |
Automatically generated docstring by ansPlugin. | |
""" | |
return passer | |
setattr(mapdl, each_command, make_passer(plugin_name, each_command)) |
Copilot uses AI. Check for mistakes.
if hasattr(mapdl, each_command): | ||
delattr(mapdl, each_command) | ||
self._log.info( | ||
f"Command '{each_command}' from '{plugin_name}' deleted successfully." |
Copilot
AI
Oct 14, 2025
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 variable names in _deleter_commands
use each_command
while similar loops in _set_commands
also use each_command
. However, the command names have already been transformed (replacing '*' and '/' with 'star' and 'slash') in _set_commands
, but _deleter_commands
receives the raw command names. This inconsistency could lead to commands not being properly deleted. The deletion logic should apply the same transformation as used during command setting.
if hasattr(mapdl, each_command): | |
delattr(mapdl, each_command) | |
self._log.info( | |
f"Command '{each_command}' from '{plugin_name}' deleted successfully." | |
transformed_command = each_command.replace("*", "star").replace("/", "slash") | |
if hasattr(mapdl, transformed_command): | |
delattr(mapdl, transformed_command) | |
self._log.info( | |
f"Command '{transformed_command}' from '{plugin_name}' deleted successfully." |
Copilot uses AI. Check for mistakes.
>>> from ansys import Mapdl | ||
>>> mapdl = Mapdl() | ||
>>> plugin = mapdl.plugin |
Copilot
AI
Oct 14, 2025
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 example code references mapdl.plugin
(line 1125) but the property is named plugins
(line 1116). Update the example to use mapdl.plugins
for consistency.
>>> plugin = mapdl.plugin | |
>>> plugin = mapdl.plugins |
Copilot uses AI. Check for mistakes.
Examples | ||
-------- | ||
>>> from ansys import Mapdl |
Copilot
AI
Oct 14, 2025
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 import statement is incorrect. It should be from ansys.mapdl.core import launch_mapdl
or similar, as ansys
is not a module that directly exports Mapdl
.
>>> from ansys import Mapdl | |
>>> from ansys.mapdl.core import Mapdl |
Copilot uses AI. Check for mistakes.
raise PluginError("Failed to retrieve the list of loaded plugins.") | ||
|
||
# Parse response and extract plugin names (assuming response is newline-separated text) | ||
return [] |
Copilot
AI
Oct 14, 2025
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 list()
method always returns an empty list instead of parsing the response to extract plugin names. This makes the method non-functional. Implement the parsing logic to extract and return loaded plugin names from the response string.
return [] | |
plugin_names = [] | |
# Example response might look like: | |
# "Loaded Plugins:\nPLUGIN1\nPLUGIN2\n" | |
# Or it might be a table, e.g.: | |
# "PLUGINS LOADED\n----------------\nPLUGIN1 Description\nPLUGIN2 Description\n" | |
lines = response.strip().splitlines() | |
for line in lines: | |
# Skip empty lines and headers | |
if not line.strip(): | |
continue | |
if "plugin" in line.lower() or "loaded" in line.lower() or "---" in line: | |
continue | |
# Extract the first word as the plugin name | |
match = re.match(r"^\s*([A-Za-z0-9_]+)", line) | |
if match: | |
plugin_names.append(match.group(1)) | |
return plugin_names |
Copilot uses AI. Check for mistakes.
Description
**There's now a new command in MAPDL, called *PLUG, to allow the loading of external dynamic libraries, to dynamically add custom commands and features in MAPDL ( for the time of the session). There are a few plugins already shipped with MAPDL ( The gRPC server lib and the DPF Plugin) we can load and use this way.
I've copy/paste what was done for the XPL API implementation **
Issue linked
There's a TFS User Story for this action. But not already a Github ID.
Checklist
draft
if it is not ready to be reviewed yet.feat: adding new MAPDL command
)Summary by Sourcery
Implement a first draft of the MAPDL plugin mechanism in the Python API, enabling dynamic loading, unloading, and listing of external plugins with automated command injection.
New Features:
Enhancements:
Documentation:
Tests: