larryhastings / appeal

Command-line parsing library for Python 3.
Other
122 stars 7 forks source link

Typing loguru logger - AttributeError: 'NoneType' object has no attribute 'get_signature' #16

Open Buedenbender opened 7 months ago

Buedenbender commented 7 months ago

Hi love the approach of appeal, came here from hearing the "Python Bytes" Podcast 🎧 . I ran into a problem, that seems on the first sight pretty self-explanatory (and it might as well is 🤷 ). The error is: 'NoneType' object has no attribute 'get_signature' Below is a MRE of the error. It stems from giving the type loguru.logger (which is an instance of loguru._Logger Logger class) to the function parameter log.

MRE

import appeal
from loguru import logger

app = appeal.Appeal()

@app.command()
def hello_world_logger(log: logger):
    log.info("Hello World")

if __name__ == "__main__":
    app.main()

The Error

   File "/.../python3.10/site-packages/appeal/argument_grouping.py", line 138, in __init__
     self.converter = Function(callable, default, name, collapse_degenerate=collapse_degenerate, signature=signature)
   File "/.../python3.10/site-packages/appeal/argument_grouping.py", line 172, in __init__
     for name, p in signature(parameter).parameters.items():
   File "/.../python3.10/site-packages/appeal/__init__.py", line 2277, in signature
     signature = cls.get_signature(p)
 AttributeError: 'NoneType' object has no attribute 'get_signature'

In search of a solution

I tried to circumvent it by changing the type hint from loguru import _logger and give the parameter log the type _logger.Logger, resulting in ImportError: cannot import name '_loguru' from 'loguru' (.../python3.10/site-packages/loguru/__init__.py) This was not to surprising, as the intention to have the module (loguru) private was obvious from the prefix `` , but I was not even aware that it is possible to have submodules not importable. However this is more related to loguru, then to appeal, back to the topic:

Is there any way to make appeal work with the loguru.logger type hint?

larryhastings commented 7 months ago

The error message is obviously terrible, but the basic problem is that loguru.logger isn't callable.

Python 3.12.0 (main, Oct 12 2023, 05:13:37) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import loguru
>>> loguru.logger
<loguru.logger handlers=[(id=0, level=10, sink=<stderr>)]>
>>> callable(loguru.logger)
False
>>> loguru.logger()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'Logger' object is not callable
>>> 

The idea with Appeal is that you pass in a callable object, and tell Appeal you want it to call the callable for you, and Appeal figures out the proper arguments and calls it. That means figuring out what to do for each parameter of the callable. Appeal has six strategies for figuring out how to fulfill a parameter, but most of them have to do with analyzing default values; if an object is annotated, that takes precedence, but the annotation has to be callable.

If the annotation for a parameter isn't callable, Appeal will fail every time. Here's another example:

import appeal

app = appeal.Appeal()

@app.command()
# annotated "log" with "3" -+
#                           |
#                           v 
def hello_world_logger(log: 3):
    log.info("Hello World")

if __name__ == "__main__":
    app.main()

This fails in the same way as your sample program, and for the same reason.

I'm not sure what you're hoping to achieve by annotating a parameter with an un-callable object and pointing Appeal at it. If you want Appeal to construct a loguru._Logger object, annotate the parameter with that class. If you want to literally pass in loguru.logger to that parameter, and also use Appeal, your best bet would be to make a partial object with functools.partial, pre-binding the desired parameter to loguru.logger, and then point Appeal at the partial object. If you had in mind something else you're going to have to describe it.

So this isn't an Appeal bug per se. But I'm going to leave this issue open as a reminder to myself to

  1. improve the error message, and
  2. add a unit test that checks that the improved error message is working.

p.s. I didn't realize Appeal got mentioned on a podcast! Can you point me at the episode?

Buedenbender commented 7 months ago

Hi, sure thing here is the podcast episode 361. Thanks already for looking into this! This absolutely makes sense and just gives me a pointer to go ahead and refactor again. The initial reason to have the logger as an argument is that the function is not only called from CLI (or a dvc pipeline to be more precise) but also from other scripts. Having loguru.logger as argument was intended to have the ability that the function can continue to write to a log file from the calling script. As I write this I realize how confusing this sounds which always a good starting point to realize it is time to refactor things 😄.

PS: From my point of view this can be closed but as you mentioned you want to keep it open, I did not close it.