EnzymeML / PyEnzyme

🧬 - Data management and modeling framework based on EnzymeML.
BSD 2-Clause "Simplified" License
21 stars 9 forks source link

change import of thinlayers? #34

Closed fbergmann closed 2 years ago

fbergmann commented 2 years ago

I was puzzled why i when i import the COPASI thinlayer like this

from pyenzyme.thinlayers.TL_Copasi import ThinLayerCopasi

I always saw the pysces import print statements. I would have understood it, if it happened when i imported the pyenzyme.thinlayers namespace, but this puzzled me.

If this is only confusing to me that is fine, in discussions with @jmrohwer the only possibility would be of not already including the thinlayers in the thinlayers.__init__.py (there probably just the base class is fine).

What do you think?

jmrohwer commented 2 years ago

The alternative would be to remove the module availability checks (as it is these that try to import pysces or copasi): https://github.com/EnzymeML/PyEnzyme/blob/0441f29af2dd6a085cb95f9c112d798f910aee5a/pyenzyme/thinlayers/TL_Pysces.py#L19-L30 and here: https://github.com/EnzymeML/PyEnzyme/blob/0441f29af2dd6a085cb95f9c112d798f910aee5a/pyenzyme/thinlayers/TL_Copasi.py#L53-L61

However, I am not in favour of that, as I think these checks fulfil a useful function.

I'm sort of 50/50 on this one. I think that the shorter import is convenient:

from pyenzyme.thinlayers import ThinLayerPysces

versus

from pyenzyme.thinlayers.TL_Pysces import ThinLayerPysces

but it is not a train smash to write the longer import. The problem does not occur if a user only has Copasi installed and not PySCeS. And if they do have PySCeS installed, one can assume that they know a little bit about it, and the startup messages can be silenced with a configuration option, as I already mentioned to Frank (https://pyscesdocs.readthedocs.io/en/latest/userguide_doc.html#configuration).

fbergmann commented 2 years ago

How about a compromise

diff --git a/pyenzyme/thinlayers/TL_Pysces.py b/pyenzyme/thinlayers/TL_Pysces.py
index 7f96204..c952cfa 100644
--- a/pyenzyme/thinlayers/TL_Pysces.py
+++ b/pyenzyme/thinlayers/TL_Pysces.py
@@ -16,9 +16,14 @@ from typing import Union, Optional
 from pyenzyme.thinlayers.TL_Base import BaseThinLayer
 from pyenzyme.enzymeml.core.exceptions import SpeciesNotFoundError

+import io
+from contextlib import redirect_stdout
+
 _PYSCES_IMPORT_ERROR = None
+_PYSCES_IMPORT_STREAM = io.StringIO()
 try:
-    import pysces
+    with redirect_stdout(_PYSCES_IMPORT_STREAM):
+        import pysces
     import lmfit
 except ModuleNotFoundError as e:
     _PYSCES_IMPORT_ERROR = """
@@ -38,7 +43,12 @@ class ThinLayerPysces(BaseThinLayer):
         measurement_ids: Union[str, list] = "all",
         init_file: Optional[str] = None,
     ):
-
+        # first time a pysces thinlayer is created, print the import messages
+        global _PYSCES_IMPORT_STREAM
+        if _PYSCES_IMPORT_STREAM is not None:
+            print(_PYSCES_IMPORT_STREAM.getvalue())
+            _PYSCES_IMPORT_STREAM = None
+
         # check dependencies
         if _PYSCES_IMPORT_ERROR:
             raise RuntimeError(_PYSCES_IMPORT_ERROR)

This code would capture the output of the import statement, and print it when the first pysces thinlayer is constructed. That way we have both:

jmrohwer commented 2 years ago

Looks good to me, have you pushed the change?

fbergmann commented 2 years ago

can do

jmrohwer commented 2 years ago

Closing this because the commit fixed it.