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

Fix mamba log level #1279

Merged
merged 2 commits into from
Nov 19, 2021
Merged

Conversation

adriendelsalle
Copy link
Member

Description

Fix mamba log level (both default and set from verbosity):

  • set log level in Context ctor
  • remove useless set_verbosity method
  • add set_log_level method
  • add python bindings for set_log_level and log levels enum (spdlog::level::level_enum)
  • use set_log_level in mamba

@wolfv
Copy link
Member

wolfv commented Nov 19, 2021

Hmm this changes api completely which forces us to do a 0.19 release .

It's not possible to keep the set verbosity which is common "conda lingo"

@adriendelsalle
Copy link
Member Author

I can keep the set_verbosity in libmamba which just set the verbosity value to just have augmented API

@adriendelsalle adriendelsalle added this to the 2021.11.19 milestone Nov 19, 2021
@wolfv
Copy link
Member

wolfv commented Nov 19, 2021

why can we not do the computations for loglevel inside c++ and have a simple set_verbosity function that handles this? I don't understand the need for all these complications and API changes.
For the user nothing has changed, just that we use spdlog instead of our previous custom logging so we should be able to follow exactly the same logic.

@adriendelsalle
Copy link
Member Author

The set_verbosity should only set the verbosity, not the log_level.
As a fallback for the log_level (when not set), mamba::detail::log_level_fallback_hook is used to have a log_level guessed from the verbosity.

For the user, nothing changed but the implementation has been improved to set verbosity and log_level separately.
Thus the need to augment the API (no breaking change) to expose a way to set the log_level.
The new util added mamba's side is nothing more than implementing Python's side mamba::detail::log_level_fallback_hook and exposing an implementation detail is not that great.

So either I move that helper outside the detail namespace, bind it, and use it in mamba, or we just keep it as it is.

@wolfv
Copy link
Member

wolfv commented Nov 19, 2021

It still breaks for users who have been using set_verbosity -- suddenly they see info logs which was not the case before (as we have experienced here). So I think we should also change the default log level to be warning again.

I don't mind the augmented API but I think it's not great to just change these things.

@wolfv
Copy link
Member

wolfv commented Nov 19, 2021

For me verbosity and log level are also very much related.

If I set a high verbosity I expect to see more logs. So for me it's a simplified way of just turning up the amount of logs that I see.

setting verbosity should just increase logs across the board.

setting log level can do that, but more specific to just mamba.

We should also make sure that setting verbosity to -vvv still enables logs from libsolv and libcurl.

@adriendelsalle
Copy link
Member Author

It still breaks for users who have been using set_verbosity -- suddenly they see info logs which was not the case before (as we have experienced here). So I think we should also change the default log level to be warning again.

It only concerns people using directly the Python bindings, and here I would say it's an improvement to decouple console verbosity and logs' one.
It's important to be able to keep a high log level with minimal or normal console verbosity to (fairly soon) be able to point to a detailed log file when an error in thrown without having to replay a sometimes huge operation with verbosity increased.

We should also make sure that setting verbosity to -vvv still enables logs from libsolv and libcurl.

it's still the case

If I set a high verbosity I expect to see more logs. So for me it's a simplified way of just turning up the amount of logs that I see.

it's also the case when using -vs CLI flags, except if micromamba's --log-level is set (there is no equivalent in mamba)

@wolfv
Copy link
Member

wolfv commented Nov 19, 2021

Maybe the confusion is as follows:

  • I am fully for having log levels for e.g. logging to files. They should be completly independeny from "verbosity"
  • Maybe for me "verbosity" is just a way to have a quick way to activate "logging to the terminal"
  • We could/shoudl have anotehr function to say: set_logfile(somefilename.log, log_level = "INFO") and similar, completely decoupled from verbosity

So I would just extend the `set_verbosity function to also set log level as appropriate.

add set_log_level method
add python bindings for set_log_level and log levels enum
use set_log_level in mamba
@wolfv wolfv merged commit ac5a5d4 into mamba-org:master Nov 19, 2021
@adriendelsalle adriendelsalle deleted the set-log-lvl-using-ctx branch November 19, 2021 22:15
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.

2 participants