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

[python] Set log level depending on in-notebook or not, and use stdout #1150

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

johnkerl
Copy link
Member

@johnkerl johnkerl commented Mar 21, 2023

Issue and/or context:

User feedback after https://github.com/single-cell-data/TileDB-SOMA/releases/tag/1.0.0

Also involves #390.

Changes:

  • Log to stdout rather than stderr. This makes notebook cells not be pink.
  • Log by default at a higher verbosity level for notebook environments.

Notes for Reviewer:

@johnkerl johnkerl marked this pull request as ready for review March 21, 2023 23:48
@johnkerl johnkerl changed the title [python] Set log level depending on in-notebook or not [python] Set log level depending on in-notebook or not, and use stdout Mar 21, 2023
@johnkerl
Copy link
Member Author

We may also want (on this PR):

  • logger to stderr for non-notebooks
  • logger to stdout for notebooks

Copy link
Member

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

I still believe this is an anti-pattern. See the Python docs for an example of why: https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library.

Quoting:

When developing a library which uses logging, you should take care to document how the library uses logging - for example, the names of loggers used. Some consideration also needs to be given to its logging configuration. If the using application does not use logging, and library code makes logging calls, then (as described in the previous section) events of severity WARNING and greater will be printed to sys.stderr. This is regarded as the best default behaviour.

and

Note It is strongly advised that you do not add any handlers other than NullHandler to your library’s loggers. This is because the configuration of handlers is the prerogative of the application developer who uses your library. The application developer knows their target audience and what handlers are most appropriate for their application: if you add handlers ‘under the hood’, you might well interfere with their ability to carry out unit tests and deliver logs which suit their requirements.

Copy link
Member

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

I'd like to see that this conform to Python best practices for the non-notebook case, e.g, regarding handlers.

I do think it is fine to special case notebook UX - I understand the use case. I just don't want to see that pollute the embedded cases (e.g., the addition of the stream handler in _set_level).

@johnkerl johnkerl requested a review from thetorpedodog March 22, 2023 02:48
@johnkerl johnkerl marked this pull request as draft September 30, 2024 15:23
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

Successfully merging this pull request may close these issues.

3 participants