Skip to content
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

feat: Add an inprocess backend to pymapdl #3198

Merged
merged 8 commits into from
Jun 21, 2024
Merged

Conversation

Gryfenfer97
Copy link
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
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
Copy link

codecov bot commented Jun 21, 2024

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
40 of 41 checks passed
@Gryfenfer97 Gryfenfer97 deleted the feat/inprocess-backend branch June 21, 2024 14:07
Copy link
Collaborator

@germa89 germa89 left a comment

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.

src/ansys/mapdl/core/mapdl_inprocess.py Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_inprocess.py Show resolved Hide resolved
self._backend = backend
self._cleanup: bool = True
self._name: str = "MapdlInProcess"
self._session_id: Optional[str] = None
Copy link
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
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
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
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
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.

src/ansys/mapdl/core/mapdl_inprocess.py Show resolved Hide resolved
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