search

python-microscope / microscope

Python library for control of microscope devices, supporting hardware triggers and distribution of devices over the network for performance and flexibility.
https://www.python-microscope.org
GNU General Public License v3.0
67 stars 39 forks source link

logging of DeviceServer objects is broken #180

Closed dstoychev closed 3 years ago

dstoychev commented 3 years ago

Commit 22a136e660e725ba850aa5d2f08217bee2d55f63 removes the setting of the logging level of the DeviceServer, which inhibits important information such as the uri of the served device. I think it probably makes most sense to add a new argument to DeviceServer for specifying the logging level.

carandraug commented 3 years ago

You're right. This works in Linux because the default multiprocessing start method is "spawn" and the child process inherits the logging level that was set on the parent. I can reproduce the issue in Linux if I force the start method to be "spawn".

I wouldn't say the device server is broken though, it's only the print of the URI that is missing (which is not essential since that's derived from the settings anyway) and the device server works fine. (strike that, I misread the title)

Regarding the fix, it seems the right approach. We would need more than just setting the logging level, we also need a way to configure and set the logging handlers which is curently done in DeviceServer but should be done by the program. The logic for that is that it's the job of the program, and not the library, to do that level of configuration so it integrates with the logging done by other libraries (imagine that all libraries that we use, like numpy and whatever, each wrote to their own files and applied their own filters --- we wouldn't be able to mix them in the device-server).

I'm not sure what the best approach to do this is, but since at the moment we only need the setting of logging level, go ahead with that function.

carandraug commented 3 years ago

Your proposal (from the multiple commits on your branches that reference this issue) of passing the whole args to the DeviceServer seems the correct way to go. I have also thought of adding new methods to DeviceServer but maybe that won't scale. However, instead of passing an argparse.Namespace, which is effectively a dict, has arbitrary keys, and tricky for outside callers of DeviceServer to replicate, I think it'll be better to have a dedicated struct for these options (maybe a dataclass -- which also requires bumping Python requirements to Python 3.7).

carandraug commented 3 years ago

I have spoken with @dstoychev and he agrees with the contents but warned of issues he found when passing the argparse.Namespace to Process:

[15:55] Danail Stoychev I had issues with the Namespace object and basically if I saved it as an attribute to the class instance (inside init()) then I would not have access to it in run() or some such. I think something was overwriting it or some such. [15:56] Danail Stoychev Which is why I ended up saving all key-value pairs of the dict manually as separate attributes. For example, instead of the DeviceServer instances having self._args, I had to make it so that they have self._logging_level, etc.

I'm proposing 938a4430c808b518f64ea4 (180-deviceserver-config branch on my fork) which is an implementation of my last comment and very similar in spirit to what @dstoychev done. It makes the required changes to microscope.win32 (even though at this moment no one knows whether it works at all) and it also makes us dependent on Python 3.7.

I have tested in Linux by forcing the multiprocessing start method to be spawn but can someone with a real Windows system give it a go?

dstoychev commented 3 years ago

Tested 938a443 on Windows and it works!

carandraug commented 3 years ago

Thanks for reviewing. I rebased and merged into master. Closing as fixed