feat: First tentative for Plugin Mapdl Mechanism python API #3627
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 😃 . |
germa89
approved these changes
Jan 10, 2025
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3627 +/- ##
==========================================
- Coverage 91.73% 91.28% -0.46%
==========================================
Files 187 190 +3
Lines 15042 15762 +720
==========================================
+ Hits 13799 14388 +589
- Misses 1243 1374 +131 🚀 New features to boost your workflow:
|
|
I'm having a look at this PR.
|
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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Comment on lines
+109
to
+110
| def passer(self, *args, **kwargs): | ||
| return self.run(*args, **kwargs) |
There was a problem hiding this comment.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
draftif 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: