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

[Cli 2.0]: Use Python loggers for output? #7502

Merged
merged 61 commits into from Oct 28, 2020

Conversation

czoido
Copy link
Contributor

@czoido czoido commented Aug 5, 2020

Changelog: Feature: Use Python loggers for Conan output in cli 2.0.
Docs: omit

This PR is just a proof of concept about the idea of changing all Conan output information messages to a Python logger. This way, the output that is passed in the ConanApi is a ConanOutput instance and the output that the commands receive for displaying the results is a CliOutput (this code needs some refactoring, also, probably the names of the classes need to be changed).

  • ConanOutput will use loggers under the hood, if the ConanApi is instantiated outside the Conan application those loggers could be configured (they have NullHandlers as default). In the Conan App all this output goes to stderr. The idea is that in Conan 2.0 the current logger is integrated in this output, probably most of the messages that the current logger is logging will be passed to the DEBUG level of this new output. For all of those remaining we will have to consider if they pass to other levels or just disappear. The ConanOutput is missing a Handler for outputing to a file yet.

If the ConanAPIV2 is created without passing an output a quiet output with the NullHandler will be set as default and the users of the API won't see the conan output messages:

api = ConanAPIV2()
api.user_list()

But still pending to manage are the progress bars, as these are handled by tqdm right now. We still have to add custom classes for progress bars that won't output them in certain cases (ci, using Conan API directly...)

  • CliOutput is just a wrapper for printing with color and is the one that is passed to the formatters of the commands for the cli2.0. This one is redirected to stdout, so we can pipe commands to files or other commands (e.g. conan user list --output=json > users.json)

There's still lots of things to consider regarding this, and the creation of ConanApp and the relation with ConanAPI. The purpose of this PR is just to discuss if passing all the output messages from conan through a logger could be useful and is not problematic.

Also, there is some code to test compatibility with tqdm library (not yet integrated with Conan custom bars) in the user_list method of the API. Note that all the output of the progress bars will go to stderr.

Other things to consider:

For testing a context manager is used to handle the output, sys.stdout and sys.stderr are redirected to a class that inherits from StringIO to get the value of the output in the tests. Now both of them are redirected to the same place but they could be redirected separate.

Things to do:

  • Regarding warnings, a "py.warnings" logger can be configured in ConanOutput so all warnings through the code can be captured by the logger. This will be adressed in a separate PR.
  • ...

#TAGS: slow

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

I like it! I have some comments, but I think this is the way to go.


CliOutput, does it make sense to send something to stderr from the command outputs? I think they also need the ConanOutput to be able to send debug/warning/info information to the user.


If tqdm is considered in a handler and not as the stream, what happens when the stream is a file object? What is written to the file?


class ConanOutput(object):
def __init__(self, stream=None, color=False):
self._logger_name = "conan.output"
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using something that cannot be a Python module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, totally. We can find a new name, or maybe use __name__?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, __name__ should be reserved for the developer logger.

import logging

log = logging.getLogger(__name__)

...
   log.debug("Something useful to debug")
...

conans/cli/output.py Outdated Show resolved Hide resolved
conans/cli/output.py Outdated Show resolved Hide resolved
conans/cli/output.py Outdated Show resolved Hide resolved
conans/cli/output.py Show resolved Hide resolved
import time
time.sleep(0.02)
except UnicodeError:
msg = msg.encode("utf8").decode("ascii", "ignore")
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the logging library I wouldn't expect an exception being raised... is this something we can check again?

conans/client/api/conan_api.py Outdated Show resolved Hide resolved
conans/cli/output.py Show resolved Hide resolved
conans/cli/output.py Outdated Show resolved Hide resolved
@czoido
Copy link
Contributor Author

czoido commented Aug 11, 2020

CliOutput, does it make sense to send something to stderr from the command outputs? I think they also need the ConanOutput to be able to send debug/warning/info information to the user.

I think that maybe we can consider the formatters of the command just a way to print a dictionary or other object but that if you want to put something in stderr through the ConanOutput maybe you should do that in the command definition itself that has access to the conan_api.out. Maybe as we add commands we see a case like that but for the moment I think I would just leave the access to CliOutput

If tqdm is considered in a handler and not as the stream, what happens when the stream is a file object? What is written to the file?

I have tested it with files and should not be a problem as the progress bars are not a part of the logger, they also go to stderr but in the logger there will only be the messages that pass through the write method of tqdm:

conan/conans/cli/output.py

Lines 95 to 103 in 099f47b

def emit(self, record):
try:
msg = self.format(record)
tqdm.tqdm.write(msg, file=self._stream)
self.flush()
except (KeyboardInterrupt, SystemExit):
raise
except:
self.handleError(record)

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

In general, I prefer to list the parameters explicitly than to take them from the *args, **kwargs via args[0] or kwargs['<name>'], it¡s much more clearer to me. Also, when passing arguments through different functions, where some functions consume some of them (and maybe forward them) and others are just forwarded it is better to use names explicitly, it is more robust if some function signature changes (no need to pay so much attention to the order of arguments):

def help(conan_api, parser, commands, groups, *args, **kwargs):
    ...
    commands[args.command].run(parser=commands[args.command].parser, conan_api=conan_api, args=["--help", ])

if not self._subcommands:
parser_args = self._parser.parse_args(*args)
if info:
self._formatters[parser_args.output](info, conan_api.out)
self._formatters[parser_args.output](info, cli_out)
else:
arg_list, = args
subcommand = arg_list[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of taking the first from args, can we add subcommand=None to the arguments and avoid these two lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure of that, all the information to decide about the subcommands is inside this class... maybe it is more confusing if we pass a potential subcommand without knowing if that conan command accepts subcommands or not...

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand it, but for me this is like the "Voy a tener suerte" in Google search 😅 I see in the same method a couple of checks like if not self._subcommands: and if subcommand in self._subcommands:, so we are already trying to check if the command accepts subcommands or not.

IMHO a implementation like this would be more clear:

def run(self, conan_api, cli_out, parser, subcommand=None, *args, **kwargs):

    if subcommand:
        try:
            self._subcommands[subcommand].run(conan_api, cli_out, *args)
        except KeyError:
            # Fallback behavior
    else:
        info = self._method(conan_api, parser, *args, **kwargs)
        parser_args = self._parser.parse_args(*args)
        self._formatters[parser_args.output](info, cli_out)

but I'm not running this locally, so this might only work in an ideal world 😅

conans/cli/command.py Show resolved Hide resolved
conans/cli/commands/help.py Outdated Show resolved Hide resolved
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Looking good

conans/cli/output.py Outdated Show resolved Hide resolved
conans/cli/cli.py Outdated Show resolved Hide resolved
conans/test/utils/tools.py Outdated Show resolved Hide resolved
conans/test/utils/tools.py Outdated Show resolved Hide resolved
@czoido czoido added this to the 1.31 milestone Oct 28, 2020
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

I think it is an improvement, lets merge it and we can keep building on top of it, probably discussing and reviewing more things and changing others, but definitely a step forward.

@memsharded memsharded marked this pull request as ready for review October 28, 2020 10:20
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

I really need to play with it and try to write a new command... but those are things that can wait. Let¡s merge it!

@memsharded memsharded merged commit dd6462d into conan-io:develop Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
CLI
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants