ansys / pysystem-coupling

Python API to System Coupling
https://systemcoupling.docs.pyansys.com/
MIT License
7 stars 4 forks source link

fix: use a named logger instead of the global logger #298

Closed nick-marnik closed 6 months ago

nick-marnik commented 6 months ago

Context: We use pysystem-coupling for the Volitare solution. Volitare is a SAF-based solution. GLOW provides logging infrastructure for us to use the native Python logging module to write to SAF log files. We found an issue where specific logging levels (debug, info) were not writing to the SAF log files.

After a deep investigation, I found the culprit to be this block of code. Specifically, line 66, combined with the fact that the default value for level on line 57 is ERROR, means that the log level for the global Python logger is set in any Python script where SyC is imported. You can see what that looks like for Volitare here

Caveat: I do not know how to test SyC - and I don't know if this change has other implications as I do not understand your architecture. If this change is not appropriate, please propose an alternative, as the current implementation isn't correct.

EDIT: For any others who encounter this issue, here is our temporary workaround in Volitare to manually fix the global logging level. This code block needs to be added to any file that imports pysystem-coupling.

nick-marnik commented 6 months ago

I suspect the failed workflows are because this PR uses my own fork. I don't have write access to this repository, so I was unable to create a branch against origin.

nick-marnik commented 6 months ago

I know that I initially approved this, but I have been having a look around some other repos, such as pyfluent and pymapdl. I suspect that these originally had the same problem as I used them for guidance when starting PySystemCoupling. It seems that the usual practice is to use something based on the "pyansys project name" of the product. So rather than use the module name (__name__), can I suggest that we use "pysystem-coupling" instead?

Updated - using __name__ comes from GLOW, but that isn't necessarily consistent with traditional pyansys packages. I'm fine with hard-coding 👍