Skip to content

feat: Add an inprocess backend to pymapdl - #3198

Merged
Gryfenfer97 merged 8 commits into
mainfrom
feat/inprocess-backend
Jun 21, 2024
Merged

feat: Add an inprocess backend to pymapdl#3198
Gryfenfer97 merged 8 commits into
mainfrom
feat/inprocess-backend

Conversation

@Gryfenfer97

Copy link
Copy Markdown
Contributor

Description

This PR add a new backend that will be used by MAPDL when exposing a python interpreter with PyMapdl.
It takes a module containing the function run_command as an argument.

@Gryfenfer97
Gryfenfer97 requested a review from a team as a code owner June 21, 2024 13:22
@ansys-reviewer-bot

Copy link
Copy Markdown
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@Gryfenfer97 Gryfenfer97 changed the title Add an inprocess backend to pymapdl feat: Add an inprocess backend to pymapdl Jun 21, 2024
@codecov

codecov Bot commented Jun 21, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes missing coverage. Please review.

Project coverage is 84.21%. Comparing base (7c1eb1e) to head (7127276).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3198      +/-   ##
==========================================
- Coverage   86.63%   84.21%   -2.43%     
==========================================
  Files          52       53       +1     
  Lines        9550     9625      +75     
==========================================
- Hits         8274     8106     -168     
- Misses       1276     1519     +243     

@Gryfenfer97
Gryfenfer97 merged commit 6d641d7 into main Jun 21, 2024
@Gryfenfer97
Gryfenfer97 deleted the feat/inprocess-backend branch June 21, 2024 14:07

@germa89 germa89 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you Jamil for this PR.

I have some concerns.

  • There is no testing. There already docker images v25.1 so if that image has the backend changes you want, you could use it.
  • It is not clear to me where run_command will be implemented, and how.
  • Many of the methods you implemented should be on _MapdlCore class to avoid code duplicity with MapdlGrpc class.

Comment thread src/ansys/mapdl/core/mapdl_inprocess.py
Comment thread src/ansys/mapdl/core/mapdl_inprocess.py
self._backend = backend
self._cleanup: bool = True
self._name: str = "MapdlInProcess"
self._session_id: Optional[str] = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This line should be implemented in _MapdlCore. My fault.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems to be something specific to the grpc backend.
We can try to move it to mapdl_grpc instead and replace these lines by a _before_run() command implemented in the children.
@koubaa

self._cleanup: bool = True
self._name: str = "MapdlInProcess"
self._session_id: Optional[str] = None
self._mute: bool = False

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Probably this one could be also implemented in _MapdlCore.

Comment on lines +52 to +61
@property
def name(self) -> str:
return self._name

@name.setter
def name(self, name) -> None:
self._name = name

def _check_session_id(self) -> None:
pass

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should go to _MapdlCore.

Comment on lines +63 to +65
def __repr__(self):
info = super().__repr__()
return info

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We are not changing anything from super() I guess it should be implemented (again) in _MapdlCore class.

Comment thread src/ansys/mapdl/core/mapdl_inprocess.py
Gryfenfer97 added a commit that referenced this pull request Jun 21, 2024
* add an inprocess backend to pymapdl

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: pyansys-ci-bot <pyansys.github.bot@ansys.com>
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.

4 participants