Skip to content
New issue

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

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

Already on GitHub? # to your account

HPI_LOGS envvar no longer works #305

Closed
purarue opened this issue Sep 6, 2023 · 7 comments
Closed

HPI_LOGS envvar no longer works #305

purarue opened this issue Sep 6, 2023 · 7 comments

Comments

@purarue
Copy link
Contributor

purarue commented Sep 6, 2023

Seems got removed when hpi switched to colorlog, is there plans to move it to some part of the my.config.core?

Otherwise I like having the ability to quickly introspect the cachew hash using it, can make a PR to re-add

@purarue
Copy link
Contributor Author

purarue commented Sep 6, 2023

Ah, I guess theres the LOGGING_LEVEL_ prefixes now, but not sure if it lets you configure everything. The --debug flag in the cli hasnt changed here

Not sure if LOGGING_LEVEL should be added as a environment variable, or maybe LOGGING_LEVEL_hpi?

Would need to update the docs as well since it still mentions HPI_LOGS

@karlicoss
Copy link
Owner

Ah sorry I've been meaning to document but got distracted! I'll take a look later today!

@purarue
Copy link
Contributor Author

purarue commented Sep 6, 2023

Am glad to PR some changes the google takeout parser/browserexport modules which previously used the HPI_LOGS to update those packages' log levels as well (I'd probably just use make_logger/LazyLogger to create the logger for each module and then check the loggers computed level to)

Though, not sure what to do about the --debug flag here, I guess could just do a:

import .logging as log_module
log_module.DEFAULT_LEVEL = 'DEBUG'

Alternatively, could change that line in the logging module to DEFAULT_LEVEL = os.environ.get("LOGGING_LEVEL", "INFO"), that would act pretty similarly to HPI_LOGS in the past (could even add a check for HPI_LOGS to keep backwards compatibility)

@karlicoss
Copy link
Owner

So there are a few things here (none of which I properly documented, sorry 🙈 )

HPI changes

  • added a helper to get logging level from an env variable, so it's easy to adjust per-module

    HPI/my/core/logging.py

    Lines 78 to 81 in c283e54

    def get_env_level(name: str) -> Level | None:
    PREFIX = 'LOGGING_LEVEL_' # e.g. LOGGING_LEVEL_my_hypothesis=debug
    # shell doesn't allow using dots in var names without escaping, so also support underscore syntax
    lvl = os.environ.get(PREFIX + name, None) or os.environ.get(PREFIX + name.replace('.', '_'), None)

  • only set logger level if it's not been set before

    HPI/my/core/logging.py

    Lines 104 to 106 in c283e54

    if logger.level == logging.NOTSET:
    # if it's already set, the user requested a different logging level, let's respect that
    logger.setLevel(lvl)

    this is more useful if you call HPI stuff in a script or other tool, that way you can configure loggers first and then call HPI stuff
    e.g. logging.getLogger('ghexport.dal').setLevel(logging.ERROR)

  • there is an experimental ENLIGHTEN_ENABLE flag, which does some magic to render a progress bar instead of dumping tons of logger messages
    Peek 2023-09-06 23-29 hpi enlighten demo
    It's a bit of work in progress, has some hacks and requires cooperation with the modules that want to benefit from it though

  • and yeah -- totally forgot about HPI_LOGS and broke it
    I guess it would be useful to keep it backwards compatible, I think it used to mean what DEFAULT_LEVEL means now?

    DEFAULT_LEVEL = 'INFO'

  • For configuring loggers, could add some config settings I guess, but considering it's a bit up in the air, didn't want to change the configs structure etc.
    So for now added a hack to load an _init_hook.py file which can be put next to the config module

    HPI/my/core/__init__.py

    Lines 42 to 47 in c283e54

    # you could put _init_hook.py next to your private my/config
    # that way you can configure logging/warnings/env variables on every HPI import
    try:
    import my._init_hook # type: ignore[import]
    except:
    pass

    E.g. mine looks like this atm

    def configure_warnings() -> None:
        IGNORED = [
            ('script is DEPRECATED'   , 'instapexport.dal'),
            ('export_dir.*export_path', 'my.github.ghexport'),
            ('export_dir.*export_path', 'my.reddit.rexport'),
            ("Specifying modules' dependencies in the config.*is deprecated", 'my.instapaper'),
            ('modify your reddit config to look like', 'my.reddit'),
        ]
    
        import warnings
        for rgx, module_name in IGNORED:
            warnings.filterwarnings('ignore', '.*' + rgx + '.*', module=module_name)
    
    
    configure_warnings()
    
    
    def configure_env() -> None:
        import os
        os.environ['ENLIGHTEN_ENABLE'] = 'true'
    
    
    configure_env()
    

cachew changes

As an example, rescuetime module doesn't pass explicit logger, but cachew just reuses my.rescuetime logger

HPI/my/rescuetime.py

Lines 31 to 32 in c283e54

@mcachew(depends_on=inputs)
def entries() -> Iterable[Res[Entry]]:

$ hpi query -s my.rescuetime.entries >/dev/null
[INFO    2023-09-06 23:06:00,710 my.rescuetime __init__.py:1005] my.rescuetime:entries: loading 685558 objects from cachew (sqlite /home/karlicos/.cache/my/my.rescuetime:entries)

(the __init__.py is kinda confusing though -- it comes from cachew, not my.rescuetime)

If you run it like this

LOGGING_LEVEL_my_rescuetime=DEBUG hpi query -s my.rescuetime.entries >/dev/null

you'll get the full cachew logs as well

LOGGING_LEVEL_my_rescuetime=DEBUG hpi query -s my.rescuetime.entries >/dev/null
[DEBUG   2023-09-06 23:09:44,218 my.rescuetime __init__.py:793 ] [my.rescuetime:entries] using inferred type typing.Union[rescuexport.dal.Entry, Exception]
[DEBUG   2023-09-06 23:09:44,219 my.rescuetime __init__.py:958 ] using /home/karlicos/.cache/my/my.rescuetime:entries for db cache
[DEBUG   2023-09-06 23:09:44,224 my.rescuetime __init__.py:962 ] new hash: {"cachew": "0.12.1.dev9+dirty", "schema": "[Column('_cachew_union_repr__Entry_is_null'
...

The downside I guess is that you'll get all DEBUG logs from my.rescuetime if there are any, but I figured it's a reasonable compromise to simplify things.

Happy to get your input and suggestions!

@purarue
Copy link
Contributor Author

purarue commented Sep 6, 2023

The automatic using of logger with cachew is really cool, I've always been a bit annoyed I had to create a logger just to ass it to cachew 👍

On the import my._init_hook, yeah, nice to separate that from the config I suppose. I've always had the import my.config at the top of every module, and if I want to do some global config I just do it in there. So having the config import at the top just handles any custom env config as a sort of side effect, but that may not be obvious to someone looking at the code for the first time.

I dont think I'll document the init hook (yet), will try it myself and see if there are any rough edges first

ENLIGHTEN_ENABLE

Wow, will definitely enable for myself, looks great :)

I think it used to mean what DEFAULT_LEVEL means now

Yeah, I think so. Thanks for the explanations on the logging, about what I made sense of from reading it over. Can make a PR to maintain backwards comparability and add some docs (in MODULE_DESIGN.org maybe?)

@karlicoss
Copy link
Owner

I've always been a bit annoyed I had to create a logger just to ass it to cachew

I think a caveat is that you'll still need to create a logger (even if you don't pass it to cachew explicitly), otherwise it falls back on the default cachew's logger. https://github.com/karlicoss/cachew/blob/b867166691572ece8f2bc3b671f8e24e4a6ce162/src/cachew/__init__.py#L759
Was thinking about creating a logger automatically, but feels a bit intrusive, so not sure about it yet

I dont think I'll document the init hook (yet), will try it myself and see if there are any rough edges first

Yep agreed -- I mainly wanted it to suppress some warnings at first, but later found out that warnings might actually need some rethinking completely because they are a bit messed up in python
https://memex.zulipchat.com/#narrow/stream/284580-python/topic/duplicate.20warnings.20in.20python

Can make a PR to maintain backwards comparability and add some docs (in MODULE_DESIGN.org maybe?)

Sounds good, will appreciate it!

@purarue
Copy link
Contributor Author

purarue commented Sep 7, 2023

resolved by #307

@purarue purarue closed this as completed Sep 7, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants