Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logger detected as abstract method by ABC #229

Closed
jansegre opened this issue Oct 7, 2019 · 5 comments
Closed

Logger detected as abstract method by ABC #229

jansegre opened this issue Oct 7, 2019 · 5 comments

Comments

@jansegre
Copy link

@jansegre jansegre commented Oct 7, 2019

I recently stumbled with this. Makes no sense to me, but seems quite easy to reproduce:

import abc
import structlog

class Foo(abc.ABC):
    log = structlog.get_logger()

Foo()  # TypeError: Can't instantiate abstract class Foo with abstract methods log

I have no idea why Python detects log as an abstract method, but this is clearly wrong. I'm still looking for a workaround.

@jansegre
Copy link
Author

@jansegre jansegre commented Oct 7, 2019

Alright, I have a workaround, but it's not pretty:

import abc
import structlog

class classproperty:
    def __init__(self, f):
        self.f = f

    def __get__(self, obj, owner):
        return self.f(owner)

_foo_log = structlog.get_logger()


class Foo(abc.ABC):
    @classproperty
    def log(cls):
        return _foo_log

Foo.log  # works
Foo().log  # also  works

(getter only classproperty from https://stackoverflow.com/a/5192374/947511)

I won't be trying to look for the reason this is happening since I found this workaround. But this is probably something that should be fixed (not sure if it's a structlog or abc bug though).

@hynek
Copy link
Owner

@hynek hynek commented Oct 14, 2019

I think it's being confused by the fact, that our generic BoundLogger will propagate any call, so the introspection fails.

This is what IPython introspects out of a BoundLoggerLazyProxy, although it's not callable at all and partial is used only within the actual BoundLogger:

Call signature: x(event=None, **event_kw)
Type:           BoundLoggerLazyProxy
String form:    <BoundLoggerLazyProxy(logger=None, wrapper_class=None, processors=None, context_class=None, initial_values={}, logger_factory_args=())>
File:           ~/.local/venvs/structlog/lib/python3.7/functools.py
Docstring:
Instantiates a BoundLogger on first usage.

Takes both configuration and instantiation parameters into account.

The only points where a BoundLogger changes state are bind(), unbind(), and
new() and that return the actual BoundLogger.

If and only if configuration says so, that actual BoundLogger is cached on
first usage.

.. versionchanged:: 0.4.0
    Added support for `logger_factory_args`.
Call docstring:
partial(func, *args, **keywords) - new function with partial application
of the given arguments and keywords.

Is there any chance you could move the get_logger into the module and then bind()/new() on instantiation? (do not bind in the class body, or that logger won't have the logging settings on itself)

@jansegre
Copy link
Author

@jansegre jansegre commented Oct 14, 2019

That's exactly what I had initially. But because of #126 (specifically the logger being picklable, and I'm using multiprocessing on some cases, which needs pickable instances), and because I prefer using the latest release (and the fix for #126 isn't released yet, though I tested the master branch and it works for me) I moved to the class from the instance when migrating these loggers to structlog (I'm slowly migrating to structlog).

But also, I have a large number of these instances. I haven't measured, so this is just speculation, but I worry that having the loggers on the instances might have a significant memory footprint. So that's another reason for moving to the class.

@hynek
Copy link
Owner

@hynek hynek commented Jan 25, 2020

I think I've fixed it in master; please report back if the problem isn't solved.

But I still think you shouldn't do it and I've made everything in structlog pickleable in the last release. :)

@jansegre
Copy link
Author

@jansegre jansegre commented Jun 2, 2020

Took me a while to touch back on this. But I can confirm it is working for me. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants