Skip to content

feat: let pylance use sub-level logger of logging#3206

Merged
wjones127 merged 1 commit intolance-format:mainfrom
yanghua:3195-pylance-logger
Dec 6, 2024
Merged

feat: let pylance use sub-level logger of logging#3206
wjones127 merged 1 commit intolance-format:mainfrom
yanghua:3195-pylance-logger

Conversation

@yanghua
Copy link
Copy Markdown
Collaborator

@yanghua yanghua commented Dec 5, 2024

Closes #3195

@yanghua yanghua force-pushed the 3195-pylance-logger branch 3 times, most recently from 41eb453 to a437e7d Compare December 5, 2024 09:25
@yanghua
Copy link
Copy Markdown
Collaborator Author

yanghua commented Dec 5, 2024

cc @westonpace @wjones127 Please have a look, thanks.

Comment thread python/python/lance/log.py
Copy link
Copy Markdown
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems alright. Though most of our logging happens at the Rust layer. That does make me wonder if it is even that valuable to let users tweak the logger, given it only controls a small fraction of our logs.

I made a suggestion on how they could share configuration. Otherwise, I think this seems fine.

Comment thread python/python/lance/log.py Outdated
Comment thread python/python/lance/log.py Outdated
import os
from typing import Optional

ENV_NAME_PYLANCE_LOGGING_LEVEL = "PYLANCE_LOGGING_LEVEL"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a logging level for pylance defined as LANCE_LOG

https://github.com/lancedb/lance/blob/f21397d47a1c6b3998e6d3dc50a858262ec17960/python/src/lib.rs#L106-L109

However, the syntax is a bit more complex:

LANCE_LOG=[target][=][level][,...]

For example:

LANCE_LOG=lance-core=debug,info

Means provide lance-core logs at debug level, everything else at info level.

I think it might be preferable to re-use and parse that environment variable.

Something like:

def get_log_level():
    lance_log = os.environ.get("LANCE_LOG", "INFO").upper()
    lance_log = [entry for entry in lance_log.split(',')
                 if '=' not in entry]
    if len(lance_log) > 0:
        return lance_log[0]
    else:
        return "INFO"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your explanation. Reasonable. Accept.

By the way, it seems there is no description about the env LANCE_LOG in the whole codebase(not sure about documentation in the official site).

If you agree, we should document it for users. WDYT?

@wjones127 wjones127 changed the title chore: let pylance use sub-level logger of logging feat: let pylance use sub-level logger of logging Dec 5, 2024
@github-actions github-actions Bot added the enhancement New feature or request label Dec 5, 2024
@yanghua yanghua force-pushed the 3195-pylance-logger branch from a437e7d to fe43947 Compare December 6, 2024 03:58
@yanghua yanghua force-pushed the 3195-pylance-logger branch from fe43947 to 3dc5e26 Compare December 6, 2024 04:59
Copy link
Copy Markdown
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. And agreed, we should add some documentation about the logging settings.

@wjones127 wjones127 merged commit 276a284 into lance-format:main Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore enhancement New feature or request python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pylance shoud use a sub-level logger of logging

2 participants