fix: exiting on class deletion (#3738)

* test: __del__method.

* refactor: simplifying exit

* chore: adding changelog file 3738.fixed.md [dependabot-skip]

* refactor: simplyfing for other interfaces.

* feat: exiting mapdl

* feat: adding __del__ to mapdl_console

* fix: improve job ID handling and remove atexit registration in MAPDL core

---------

Co-authored-by: pyansys-ci-bot <[email protected]>

Author: germa89

doc/changelog.d/3738.fixed.md

@@ -0,0 +1 @@
+fix: exiting on class deletion

src/ansys/mapdl/core/mapdl_console.py

@@ -27,6 +27,7 @@
 import os
 import re
 import time
+from warnings import warn
 
 from ansys.mapdl.core import LOG
 from ansys.mapdl.core.errors import MapdlExitedError, MapdlRuntimeError
@@ -274,6 +275,20 @@ def mesh(self):
         """
         return self._mesh
 
+    def __del__(self):
+        """Garbage cleaning the class"""
+        self._exit()
+
+    def _exit(self):
+        """Minimal exit command. No logging or cleanup so it does not raise
+        exceptions"""
+        if self._process is not None:
+            try:
+                self._process.sendline("FINISH")
+                self._process.sendline("EXIT")
+            except Exception as e:
+                LOG.warning(f"Unable to exit ANSYS MAPDL: {e}")
+
     def exit(self, close_log=True, timeout=3):
         """Exit MAPDL process.
 
@@ -284,12 +299,7 @@ def exit(self, close_log=True, timeout=3):
             ``None`` to not wait until MAPDL stops.
         """
         self._log.debug("Exiting ANSYS")
-        if self._process is not None:
-            try:
-                self._process.sendline("FINISH")
-                self._process.sendline("EXIT")
-            except Exception as e:
-                LOG.warning(f"Unable to exit ANSYS MAPDL: {e}")
+        self._exit()
 
         if close_log:
             self._close_apdl_log()
@@ -302,11 +312,10 @@ def exit(self, close_log=True, timeout=3):
             tstart = time.time()
             while self._process.isalive():
                 time.sleep(0.05)
-                telap = tstart - time.time()
-                if telap > timeout:
-                    return 1
-
-        return 0
+                if (time.time() - tstart) > timeout:
+                    if self._process.isalive():
+                        warn("MAPDL couldn't be exited on time.")
+                        return
 
     def kill(self):
         """Forces ANSYS process to end and removes lock file"""

src/ansys/mapdl/core/mapdl_core.py

@@ -22,7 +22,7 @@
 
 """Module to control interaction with MAPDL through Python"""
 
-import atexit
+# import atexit
 from functools import wraps
 import glob
 import logging
@@ -247,7 +247,6 @@ def __init__(
         **start_parm,
     ):
         """Initialize connection with MAPDL."""
-        atexit.register(self.__del__)  # registering to exit properly
         self._show_matplotlib_figures = True  # for testing
         self._query = None
         self._exited: bool = False
@@ -2344,21 +2343,8 @@ def exit(self):  # pragma: no cover
         raise NotImplementedError("Implemented by child class")
 
     def __del__(self):
-        """Clean up when complete"""
-        if self._cleanup:
-            # removing logging handlers if they are closed to avoid I/O errors
-            # when exiting after the logger file has been closed.
-            # self._cleanup_loggers()
-            logging.disable(logging.CRITICAL)
-
-            try:
-                self.exit()
-            except Exception as e:
-                try:  # logger might be closed
-                    if hasattr(self, "_log") and self._log is not None:
-                        self._log.error("exit: %s", str(e))
-                except ValueError:
-                    pass
+        """Kill MAPDL when garbage cleaning"""
+        self.exit()
 
     def _cleanup_loggers(self):
         """Clean up all the loggers"""

src/ansys/mapdl/core/mapdl_grpc.py

@@ -424,7 +424,7 @@ def __init__(
 
         self._busy: bool = False  # used to check if running a command on the server
         self._local: bool = start_parm.get("local", True)
-        self._launched: bool = start_parm.get("launched", True)
+        self._launched: bool = start_parm.get("launched", False)
         self._health_response_queue: Optional["Queue"] = None
         self._exiting: bool = False
         self._exited: Optional[bool] = None
@@ -1117,32 +1117,31 @@ def exit(self, save=False, force=False, **kwargs):
 
         Notes
         -----
+        If Mapdl didn't start the instance, then this will be ignored unless
+        ``force=True``.
+
         If ``PYMAPDL_START_INSTANCE`` is set to ``False`` (generally set in
         remote testing or documentation build), then this will be
         ignored. Override this behavior with ``force=True`` to always force
         exiting MAPDL regardless of your local environment.
 
+        If ``Mapdl.finish_job_on_exit`` is set to ``True`` and there is a valid
+        JobID in ``Mapdl.jobid``, then the SLURM job will be canceled.
+
         Examples
         --------
         >>> mapdl.exit()
         """
         # check if permitted to start (and hence exit) instances
         from ansys.mapdl import core as pymapdl
 
-        if hasattr(self, "_log"):
-            self._log.debug(
-                f"Exiting MAPLD gRPC instance {self.ip}:{self.port} on '{self._path}'."
-            )
+        self._log.debug(
+            f"Exiting MAPLD gRPC instance {self.ip}:{self.port} on '{self._path}'."
+        )
 
         mapdl_path = self._path  # using cached version
-        if self._exited is None:
-            self._log.debug("'self._exited' is none.")
-            return  # Some edge cases the class object is not completely
-            # initialized but the __del__ method
-            # is called when exiting python. So, early exit here instead an
-            # error in the following self.directory command.
-            # See issue #1796
-        elif self._exited:
+
+        if self._exited:
             # Already exited.
             self._log.debug("Already exited")
             return
@@ -1153,26 +1152,25 @@ def exit(self, save=False, force=False, **kwargs):
 
         if not force:
             # ignore this method if PYMAPDL_START_INSTANCE=False
-            if not self._start_instance:
-                self._log.info("Ignoring exit due to PYMAPDL_START_INSTANCE=False")
+            if not self._start_instance or not self._launched:
+                self._log.info(
+                    "Ignoring exit due to PYMAPDL_START_INSTANCE=False or because PyMAPDL didn't launch the instance."
+                )
                 return
 
             # or building the gallery
             if pymapdl.BUILDING_GALLERY:
                 self._log.info("Ignoring exit due as BUILDING_GALLERY=True")
                 return
 
-        # Actually exiting MAPDL instance
-        if self.finish_job_on_exit:
-            self._exiting = True
-            self._exit_mapdl(path=mapdl_path)
-            self._exited = True
+        # Exiting MAPDL instance if we launched.
+        self._exiting = True
+        self._exit_mapdl(path=mapdl_path)
+        self._exited = True
 
-            # Exiting HPC job
-            if self._mapdl_on_hpc:
-                self.kill_job(self.jobid)
-                if hasattr(self, "_log"):
-                    self._log.debug(f"Job (id: {self.jobid}) has been cancel.")
+        if self.finish_job_on_exit and self._mapdl_on_hpc:
+            self.kill_job(self.jobid)
+            self._log.debug(f"Job (id: {self.jobid}) has been cancel.")
 
         # Exiting remote instances
         if self._remote_instance:  # pragma: no cover
@@ -3818,20 +3816,17 @@ def __del__(self):
         """In case the object is deleted"""
         # We are just going to escape early if needed, and kill the HPC job.
         # The garbage collector remove attributes before we can evaluate this.
-        try:
-            # Exiting HPC job
-            if (
-                hasattr(self, "_mapdl_on_hpc")
-                and self._mapdl_on_hpc
-                and hasattr(self, "finish_job_on_exit")
-                and self.finish_job_on_exit
-            ):
+        if self._exited:
+            return
 
-                self.kill_job(self.jobid)
+        if not self._start_instance:
+            # Early skip if start_instance is False
+            return
 
-            if not self._start_instance:
-                return
+        # Killing the instance if we launched it.
+        if self._launched:
+            self._exit_mapdl(self._path)
 
-        except Exception as e:  # nosec B110
-            # This is on clean up.
-            pass  # nosec B110
+        # Exiting HPC job
+        if self._mapdl_on_hpc and self.finish_job_on_exit:
+            self.kill_job(self.jobid)

tests/conftest.py

@@ -658,8 +658,9 @@ def mapdl(request, tmpdir_factory):
             with pytest.raises(MapdlExitedError):
                 mapdl._send_command_stream("/PREP7")
 
-        # Delete Mapdl object
-        del mapdl
+    # Delete Mapdl object
+    mapdl.exit()
+    del mapdl
 
 
 ################################################################

tests/test_console.py

@@ -27,6 +27,8 @@
 """
 import os
 import time
+from unittest.mock import patch
+from warnings import catch_warnings
 
 import pytest
 
@@ -111,6 +113,8 @@ def test_basic_command(cleared, mapdl_console):
 def test_allow_ignore(mapdl_console, cleared):
     mapdl_console.allow_ignore = False
     assert mapdl_console.allow_ignore is False
+
+    mapdl_console.finish()
     with pytest.raises(pymapdl.errors.MapdlInvalidRoutineError):
         mapdl_console.k()
 
@@ -132,7 +136,7 @@ def test_chaining(mapdl_console, cleared):
 
 
 def test_e(mapdl_console, cleared):
-    mapdl.prep7()
+    mapdl_console.prep7()
     mapdl_console.et("", 183)
     n0 = mapdl_console.n("", 0, 0, 0)
     n1 = mapdl_console.n("", 1, 0, 0)
@@ -613,7 +617,10 @@ def test_load_table(mapdl_console, cleared):
         ]
     )
     mapdl_console.load_table("my_conv", my_conv, "TIME")
-    assert np.allclose(mapdl_console.parameters["my_conv"], my_conv[:, -1])
+    assert np.allclose(
+        mapdl_console.parameters["my_conv"].reshape(-1, 1),
+        my_conv[:, -1].reshape(-1, 1),
+    )
 
 
 def test_mode_console(mapdl_console, cleared):
@@ -647,3 +654,57 @@ def test_console_apdl_logging_start(tmpdir):
     assert "K,2,1,0,0" in text
     assert "K,3,1,1,0" in text
     assert "K,4,0,1,0" in text
+
+
+def test__del__console():
+    from ansys.mapdl.core.mapdl_console import MapdlConsole
+
+    class FakeProcess:
+        def sendline(self, command):
+            pass
+
+    class DummyMapdl(MapdlConsole):
+        @property
+        def _process(self):
+            return _proc
+
+        def __init__(self):
+            self._proc = FakeProcess()
+
+    with (
+        patch.object(DummyMapdl, "_process", autospec=True) as mock_process,
+        patch.object(DummyMapdl, "_close_apdl_log") as mock_close_log,
+    ):
+
+        mock_close_log.return_value = None
+
+        # Setup
+        mapdl = DummyMapdl()
+
+        del mapdl
+
+        mock_close_log.assert_not_called()
+        assert [each.args[0] for each in mock_process.sendline.call_args_list] == [
+            "FINISH",
+            "EXIT",
+        ]
+
+
+@pytest.mark.parametrize("close_log", [True, False])
+def test_exit_console(mapdl_console, close_log):
+    with (
+        patch.object(mapdl_console, "_close_apdl_log") as mock_close_log,
+        patch.object(mapdl_console, "_exit") as mock_exit,
+    ):
+        mock_exit.return_value = None
+        mock_close_log.return_value = None
+
+        with catch_warnings(record=True):
+            mapdl_console.exit(close_log=close_log, timeout=1)
+
+        if close_log:
+            mock_close_log.assert_called_once()
+        else:
+            mock_close_log.assert_not_called()
+
+        mock_exit.assert_called_once()

tests/test_mapdl.py

@@ -2715,3 +2715,53 @@ def my_func(i):
 
     for i in range(1_000_000):
         my_func(i)
+
+
+@pytest.mark.parametrize("start_instance", [True, False])
+@pytest.mark.parametrize("exited", [True, False])
+@pytest.mark.parametrize("launched", [True, False])
+@pytest.mark.parametrize("on_hpc", [True, False])
+@pytest.mark.parametrize("finish_job_on_exit", [True, False])
+def test_garbage_clean_del(
+    start_instance, exited, launched, on_hpc, finish_job_on_exit
+):
+    from ansys.mapdl.core import Mapdl
+
+    class DummyMapdl(Mapdl):
+        def __init__(self):
+            pass
+
+    with (
+        patch.object(DummyMapdl, "_exit_mapdl") as mock_exit,
+        patch.object(DummyMapdl, "kill_job") as mock_kill,
+    ):
+
+        mock_exit.return_value = None
+        mock_kill.return_value = None
+
+        # Setup
+        mapdl = DummyMapdl()
+        mapdl._path = ""
+        mapdl._jobid = 1001
+
+        # Config
+        mapdl._start_instance = start_instance
+        mapdl._exited = exited
+        mapdl._launched = launched
+        mapdl._mapdl_on_hpc = on_hpc
+        mapdl.finish_job_on_exit = finish_job_on_exit
+
+        del mapdl
+
+        if exited or not start_instance or not launched:
+            mock_exit.assert_not_called()
+        else:
+            mock_exit.assert_called_once()
+
+        if exited or not start_instance:
+            mock_kill.assert_not_called()
+        else:
+            if on_hpc and finish_job_on_exit:
+                mock_kill.assert_called_once()
+            else:
+                mock_kill.assert_not_called()