refactor: clean mapdl inprocess and move mute to MapdlCore

[Gryfenfer97]

Description

Cleanup of MapdlInProcess and light refactor

This PR is a followup to the suggestion made in #3198

Points to address:

• move the name getter in the backend
• add _before_run() and _after_run() to let the backend do specific tasks
  • move _session_id to mapdl_grpc
• add tests (?)

[ansys-reviewer-bot]

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

@ansys-reviewer-bot review

[codecov]

Codecov Report

Attention: Patch coverage is 51.85185% with 13 lines in your changes missing coverage. Please review.

Project coverage is 85.15%. Comparing base (dfe692c) to head (f23754d).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3220      +/-   ##
==========================================
+ Coverage   85.08%   85.15%   +0.06%     
==========================================
  Files          55       55              
  Lines        9926     9925       -1     
==========================================
+ Hits         8446     8452       +6     
+ Misses       1480     1473       -7     

[Gryfenfer97]

@germa89 Some comments:

• I don't know how to test this backend as it is only callable by MAPDL
• for the code duplicity I have removed the most easy part but the name and the section ID cause some problems
  • the name in mapdl_grpc is defined in the getter ni a lazy way (see). this is incompatible with the name being defined in MapdlCore. One solution could be to remove the lazyness and define _name directly in the constructor. Then the getter can be moved to MapdlCore
  • the section_id is only used for the grpc backend but is referenced in MapdlCore (see these lines). @koubaa suggested to create two functions: _before_run and after_run() that would be defined in the children. Then, MapdlCore could just refer to this function and the session_id check could be done in MapdlGrpc

[koubaa]

@Gryfenfer97 you can test it by mocking the backend in a test

[germa89]

I don't know how to test this backend as it is only callable by MAPDL

We are deploying v251 (See #3210). I can also add the daily build (tag latest_daily) docker image. Then I guess you could test it with mapdl.input_strings or whatever you are using at the moment.
We might need to design a testing framework for this (set of helpers functions for testing *PYTHON).

If you are strictly working with MAPDL (starting MAPDL without the -grpc flag), then I would setup something different. We can use MAPDL CLI ( ansys251 -b -i input.inp -o output.out ). We will have to create a fixture that create the input file from the command lines given (you can add all the need MAPDL or Python boilerplate), and runs it using the metioned command. It should also read the output file to check the values. If the values are meant be stored in MAPDL memory, we can save an RST file and read it later. If the values are in the Python memory, we can just export the results as csv, or pickle files.
For this, I think the best is to run everything inside the MAPLD docker image, for that you will have to wait for the Ubuntu docker version (that task is on me then). The CentOS image might work too, but we had problems installing Python. You can try though.

this is incompatible with the name being defined in MapdlCore

name is dependent on each implementation and it should be implemented in all implemented in all of them. I don't see why the current implementation is incompatible. But I agree with you, let's define completely name in MapdlCore (name is just returning _name). And then implement _name in each subclass constructor.

the section_id is only used for the grpc backend but is referenced...

Assuming you mean _session_id, I'm Ok with the approach you propose.

@koubaa Mocking might not be the best for this case, if we are expecting to have tight coupling. Maintaining the mocked version might be a lot of burden.

[germa89]

Furthermore... the grpc server can be activated from inside MAPDL using an MAPDL command (ask Fred about it, and let me know).

[germa89]

I guess we might have issues when executing an /input file if the gRPC server is not active. Can the gRPC server run at the same as in-memory?

[Gryfenfer97]

I guess we might have issues when executing an /input file if the gRPC server is not active. Can the gRPC server run at the same as in-memory?

If having the grpc server running doesn't block MAPDL there is no reason to have a problem with both of them running at the same time

[germa89]

I guess we might have issues when executing an /input file if the gRPC server is not active. Can the gRPC server run at the same as in-memory?

If having the grpc server running doesn't block MAPDL there is no reason to have a problem with both of them running at the same time

Good. Test that.

Remember also, that there is another...

There is another backend... the console one. I never used it, but I think it worked by directy typing the commands in the MAPDL terminal (sort of). We do not test it, nor develop against it, so it is going to be fun if we have to activate it.

[koubaa]

I guess we might have issues when executing an /input file if the gRPC server is not active. Can the gRPC server run at the same as in-memory?

why would we have this issue? It can run at the same time but I don't think we should, at least not only for the purpose of testing pymapdl

[koubaa]

@koubaa Mocking might not be the best for this case, if we are expecting to have tight coupling. Maintaining the mocked version might be a lot of burden.

I disagree. Here's how it could work:

def test_mapdl_c_backend():
    result_container = [""]
    class MockBackend:
        def run_command(command, verbose, mute):
            return result_container[0]
    mock_backend = MockBackend()
    mapdl = MapdlInProcess(mock_backend)
    
    result_container[0]="ENTERING PREP7 PROCESSOR"
    mapdl.prep7()

I don't see why this would be hard to maintain, and this can give 100% code coverage to all the logic we add to the MapdlInProcess class

[germa89]

I guess we might have issues when executing an /input file if the gRPC server is not active. Can the gRPC server run at the same as in-memory?

why would we have this issue? It can run at the same time but I don't think we should, at least not only for the purpose of testing pymapdl

How are we sending the /INPUT command if the gRPC server is not active? It must then being through batch -b whic is OK, but not super convenient.

[germa89]

@koubaa Mocking might not be the best for this case, if we are expecting to have tight coupling. Maintaining the mocked version might be a lot of burden.

I disagree. Here's how it could work:

def test_mapdl_c_backend():
    result_container = [""]
    class MockBackend:
        def run_command(command, verbose, mute):
            return result_container[0]
    mock_backend = MockBackend()
    mapdl = MapdlInProcess(mock_backend)
    
    result_container[0]="ENTERING PREP7 PROCESSOR"
    mapdl.prep7()

I don't see why this would be hard to maintain, and this can give 100% code coverage to all the logic we add to the MapdlInProcess class

I think the method you proposed makes sense when on the client we are having some postprocessing (PyMAPDL postprocessing module for instance) or preprocessing (PyHPS for instance, building the REST request) of the server response. We fake the server response, pre/post-process it normally, and compare it with something.

But I see no reason to fake the server response when there is not much pre/post-processing. Since we are testing the backend, we need to make sure we can send commands, and that the server can process them and give something correct back. What PyMAPDL is doing with the server response is not important in testing the backend.

This approach could be interesting for testing the post-processing module. We can have MAPDL command responses and make sure that PyMAPDL processes them correctly.

[koubaa]

I guess we might have issues when executing an /input file if the gRPC server is not active. Can the gRPC server run at the same as in-memory?

why would we have this issue? It can run at the same time but I don't think we should, at least not only for the purpose of testing pymapdl

How are we sending the /INPUT command if the gRPC server is not active? It must then being through batch -b whic is OK, but not super convenient.

How it is done is not the concern of the mapdl_in_process.py. It's job is to give the pymapdl frontend using the backend protocol which it is given

[koubaa]

@koubaa Mocking might not be the best for this case, if we are expecting to have tight coupling. Maintaining the mocked version might be a lot of burden.

I disagree. Here's how it could work:

def test_mapdl_c_backend():
    result_container = [""]
    class MockBackend:
        def run_command(command, verbose, mute):
            return result_container[0]
    mock_backend = MockBackend()
    mapdl = MapdlInProcess(mock_backend)
    
    result_container[0]="ENTERING PREP7 PROCESSOR"
    mapdl.prep7()

I don't see why this would be hard to maintain, and this can give 100% code coverage to all the logic we add to the MapdlInProcess class

I think the method you proposed makes sense when on the client we are having some postprocessing (PyMAPDL postprocessing module for instance) or preprocessing (PyHPS for instance, building the REST request) of the server response. We fake the server response, pre/post-process it normally, and compare it with something.

But I see no reason to fake the server response when there is not much pre/post-processing. Since we are testing the backend, we need to make sure we can send commands, and that the server can process them and give something correct back. What PyMAPDL is doing with the server response is not important in testing the backend.

This approach could be interesting for testing the post-processing module. We can have MAPDL command responses and make sure that PyMAPDL processes them correctly.

What do you mean by server? There is no server.

[germa89]

Talking with @koubaa we did both agree that I was applying wrong logic here. The indications given by @koubaa and @Gryfenfer97 are correct from my new perspective.

@koubaa mentioned that since we are only preparing the backend, it does not need to be explicitly called MapdlInProcess. Sorry @Gryfenfer97 for asking before to change it. You can revert that if you wish.

Additionally, I believe we are still missing some "pseudo-testing". Assuming that most of the in-process backend will be tested in MAPDL CICD, I still would like to make sure that the object we are attaching to mapdl._in_process_backend has certain characteristics. For instance, it has the method run_command, returns a string, etc.

[germa89]

@Gryfenfer97 ping me when everything is ready.

[Gryfenfer97]

Also should we consider changing all the "NotImplementedError" to abstract methods ? it seems to be an anti-pattern
https://codereview.stackexchange.com/a/47115

Code Review Stack Exchange

Raising error if method not overridden by sub-class

Background

I have a base (only 2 classes inherit from it, and only from it) abstract (meaning I don't want it to be used directly) class that implements some common functionality.

Some of it depe...

[germa89]

Two things to think about it.

• Include a method (even if it is private) to check wheter yo are using MapdlInMemory or not. Same as we have mapdl.is_grpc and mapdl.is_console. We check mapdl._mode for that.
• Can someone use this backend by mistake in PyMAPDL? For instance:

mapdl = launch_mapdl(mode="inmemory")

I know it does not make sense.. so I want to make sure that no one is going to (by mistake or almost purposely) try to start this backend, and not getting a good formatter exception.

[github-actions]

Hello! 👋

Your PR is changing the image cache. So I am attaching the new image cache in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore you will see the actions showing in their status Expected — Waiting for status to be reported. Do not worry. You commit workflow is still running here 😄

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. 🤓

[germa89]

How are we doing with this @Gryfenfer97 ??