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

Wrap CMATRIX for gRPC #584

Merged
merged 12 commits into from
Sep 10, 2021
Merged

Wrap CMATRIX for gRPC #584

merged 12 commits into from
Sep 10, 2021

Conversation

germa89
Copy link
Collaborator

@germa89 germa89 commented Sep 8, 2021

Resolves issue #571 by wrapping CMATRIX internally using non-interactive. CMATRIX within MAPDL redirects the server output and simply running `mapdl.run('cmatrix...') will work.

@germa89 germa89 requested a review from akaszynski September 8, 2021 09:59
@germa89 germa89 self-assigned this Sep 8, 2021
@germa89 germa89 changed the title Fix/issue 571 WIP-Fix/issue 571 Sep 9, 2021
Answer review.
@germa89 germa89 changed the title WIP-Fix/issue 571 Fix/issue 571 Sep 9, 2021
@germa89 germa89 mentioned this pull request Sep 9, 2021
germa89 added a commit that referenced this pull request Sep 9, 2021
I rather have the test failing until we implement the correct changes.
Hence this test will fail until we implement #584.
I keep that line for testing purposes.
@akaszynski akaszynski changed the title Fix/issue 571 Wrap CMATRIX for gRPC Sep 9, 2021
@akaszynski
Copy link
Collaborator

A few notes:

PR Naming

PRs need to have descriptive names. Include the issue resolved within the PR notes and not in the title. I've changed this to match the correct style.

Wrapped CMATRIX Method

We need to run the non-interactive part within the method that we're wrapping. There's no point in wrapping a method unless we're also going to do the leg work making the method easy to run. As such, we need to change the following:
https://github.com/pyansys/pymapdl/blob/0ef7b24a53ef890ed5e0d42e7533806acd5fc654/ansys/mapdl/core/mapdl_grpc.py#L1636-L1637

to:

# The CMATRIX command needs to run in non-interactive mode as the server redirects output.
if not self._store_commands:  # check if already in non-interactive mode
    with self.non_interactive:
        super().cmatrix(symfac, condname, numcond, grndkey, capname, **kwargs)
        # Output stored in cmatrix.out
        self._response = self._download_as_raw('cmatrix.out').decode()
        return self._response 

# otherwise, simply run cmatrix as we're already in non-interactive
super().cmatrix(symfac, condname, numcond, grndkey, capname, **kwargs)

This does two things. First, we're checking if we're running in non-interactive. We don't want to enter it twice (and in fact, open a new issue to add a check to see if we're already running non-interactive when initially entering non-interactive), so if we're already non-interactive, we just fire off the command and it gets placed in the command queue. The output won't be captured there anyway, so we won't worry about returning the last response. Otherwise, we return the output.

This was implemented in commit 70b6f17.


Unit Testing

I went ahead and closed #589. New unit tests must be added in the PR that adds the new feature. It's best to practice Test Driven Development.

As such, your unit test should be in the tests/test_grpc.py file. Additionally, your unit test needs to:

  • Use existing fixtures so we avoid starting/stopping mapdl in or outside of devops
  • Be as simplistic as possible

Commit 70b6f17 also moves the unit test here. Note how we're using the mapdl fixture defined in conftest.py to manage mapdl, and then creating a new fixture to run the setup for cmatrix. Each unit test should test one method, so all the setup for the method needs to be outside of the unit test.

Also note that the unit test covers both possible branches of the method. We test if it works outside of a non-interactive context, which is how we expect the user to use it, but we also test it within the non-interactive context as it's an edge case we have to account for (albeit unlikely).

With this approach, we know that any feature we add is fully covered by our testing and we don't need a separate PR to verify that our feature works.

@akaszynski
Copy link
Collaborator

CI/CD is failing because the image appears to have an issue with running CMATRIX on the docker image. With docker running locally it's failing outright with:

MapdlInvalidRoutineError: *** WARNING ***                         CP =       1.785   TIME= 18:53:31
 CMATRIX is not a recognized SOLUTION command, abbreviation, or macro.   

I'm rebuilding the image locally to verify this.

@akaszynski
Copy link
Collaborator

Found out the error with CMATRIX. There are a ton of MAC files I've neglected to copy over to the docker image. I'm regenerating and we'll use this PR to bump the docker image.

Copy link
Collaborator

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

Ended up being a longer PR than expected, but I've discovered what was the issue with our docker images.

Please review my changes and merge if you're happy with them.

@germa89
Copy link
Collaborator Author

germa89 commented Sep 10, 2021

PR naming

PRs need to have descriptive names. Include the issue resolved within the PR notes and not in the title. I've changed this to match the correct style.

I should have realized this way before.

Wrap CMATRIX

We need to run the non-interactive part within the method that we're wrapping. There's no point in wrapping a method unless we're also going to do the leg work making the method easy to run.

To be honest, I thought about it. Quite a bit, but since other problematic commands are not being wrapped I thought the consensus was to not "hide" that from the user. Now I know I will do that from now on.

I think I mentioned somewhere before that we could create a wrapper (decorator?) for this type of functions, call it @only_non_interactive_execution for example. So we can apply it for all the functions (assuming similar issues/solutions for all of them).

Does it make sense?

Unit testing

I really need to catch up on unit testing. Will do that next days.

Docker issue:

How did you get this message?

MapdlInvalidRoutineError: *** WARNING ***                         CP =       1.785   TIME= 18:53:31
CMATRIX is not a recognized SOLUTION command, abbreviation, or macro.   

It will be good for me learn that for better debugging.

Merging

Approved.

@germa89 germa89 merged commit 7cb3d94 into main Sep 10, 2021
@akaszynski akaszynski added this to the v0.60.0 milestone Sep 10, 2021
@germa89 germa89 deleted the fix/issue_571 branch November 2, 2021 14:48
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.

2 participants