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

Include all resolution conflict fix info on stderr. #9421

Closed
wants to merge 1 commit into from

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Jan 3, 2021

This allows resolution conflict errors to be triaged completely from
stderr which generally aids non-interactive use.

Closes #9420

This allows resolution conflict errors to be triaged completely from
stderr which generally aids non-interactive use.

Closes pypa#9420
@uranusjr
Copy link
Member

uranusjr commented Jan 5, 2021

I am mostly amblivient, but switching to critical means the text would be a big block of red, which can be difficult to read. It‘d be best if we can have a way to keep this white while using stderr.

@jsirois
Copy link
Contributor Author

jsirois commented Jan 5, 2021

@uranusjr is logging directly to stderr considered kosher? I could do that instead, but that seems a bit ad-hoc.

Also, is the general principle of logging information for humans to stderr and data to act upon to stdout an accepted principle in this project? See: #9420 (comment)
If so, that's great. If not, then even with some version of this patch in - as a non-interactive Pip user I have to be wary and vigilant to make sure future changes don't send more information for humans to stdout.

@uranusjr
Copy link
Member

uranusjr commented Jan 6, 2021

Outputting directly is probably not a good way to go. Considering verbosity settings (this needs to be slienced with -q), the best way to do this is probably to somehow have a “logger.info() but to stderr”. I wonder if this is possible with custom logging configurations and context.

I don’t hold a strict stance machine-vs-human readable output separation (my personal opinion is this should be switched with a flag instead), but can’t speak for other maintainers.

@pfmoore
Copy link
Member

pfmoore commented Jan 6, 2021

I agree with @uranusjr - we should keep use of "critical" level (red text) to a minimum, as it's intended to highlight the key message, not be used for everything.

On human vs machine readable, my view is that machine-readable output should be triggered by a flag - "normal" output should always be for humans, whether on stdout or stderr. So any logging output is for humans primarily.

At a project level, I don't think pip has a very clear policy on whether particular output should go to stdout or stderr, it's basically just by logging level at the moment. For control of what gets displayed, -q and -v are the main controls, rather than the out/err split.

@jsirois
Copy link
Contributor Author

jsirois commented Jan 6, 2021

Since the discussion is happening here instead of on the issue - do either of you have any suggestions for non-interactive users then to move forward? The workaround is, I think you'd agree, pretty awful (https://github.com/pantsbuild/pex/pull/1162/files#diff-2ca3fdfe3760e769f87c679c3e1c162ec315a50b839276033a751650ec130669R386). If non-interactive use is not supported - I can live with that. If it is intended to be supported though, I'd like to move forward resolving this issue but your feedback so far has been pretty non-committal.

If this were a project I had say in, I'd champion all log output go to stderr, color aside, allowing for stdout to be used for data and pipelines. We do this in Pex for example so you can always run a Pex CLI command and pipe its stdout to - say - jq to do useful things. Logging is configured to always go to stderr regardless of log level.

If making that change sounds reasonable I'll ditch this PR and send up a new one that changes the PIP logging configuration to always use stderr. I suspect thought that that PR will raise eyebrows more than this one; so your feedback in advance is welcome.

@sbidoul
Copy link
Member

sbidoul commented Jan 6, 2021

I personally agree that all logging should go to stderr. But I guess some people rely on the current behaviour... and I don't know how to transition gracefully.

Regarding machine readable output, it is hard for pip to produce an output that is both easy to read for humans, easy to parse for machines, and with backward compatibility guarantees. And the pip install output should be optimized for humans first.

So I'd really discourage parsing the current output and go for some sort of json output for install, uninstall and list. There is some discussion about an extended json format for pip list in #8008 (mostly about how to represent metadata). And pip install/uninstall, with --dry-run (#53) or not, could report what it does in json too. There should be some up-front design of the desired output before writing code though, because once the json format is released it will be very hard to change except by adding new fields. I don't have time to drive the discussion myself in the short term, but I'd be happy to review proposals, assuming other pip maintainers agree it is a good feature to have.

@pfmoore
Copy link
Member

pfmoore commented Jan 6, 2021

Ignoring the option of using 2>&1 and not having to deal with the difference between stdout and stderr, I don't think either of us have objected to the idea of an "info level, but logs to stderr" logging level, if that helps your use case.

But I'm not clear what you want to do here. If you're parsing pip's output via a program, then no, we don't guarantee much about that. I'm not saying we don't support it, but we do expect consumers to understand that the output is intended to be for humans, and as such it can change without warning to improve (human) readability, and the job of parsing the important bits out of the output is down to the consumer.

If you want a machine-readable format of pip's log output, then we've both said we'd expect that to be handled via a new command line flag, probably requesting JSON format or something similar. That would be a significant new feature. It's not something we'd reject out of hand, but we've never had anyone else request it so there would be questions about how useful it is in relation to the amount of effort to implement it.

If you want an analogy, consider git status which is human-readable, with git status --porcelain being the machine-readable version. Pip has never had (or needed) "porcelain" versions of its commands so far, but it's not inconceivable that it might be useful.

@pfmoore
Copy link
Member

pfmoore commented Jan 6, 2021

To be clear, all this PR does is move the output to stderr (so basically, similar to what 2>&1 would offer). You've now extended the discussion to the more general question of machine readable output, which I've answered here as best I can, but if you think it's better suited to the original issue (or a new one, explicitly talking about machine readable data) then I'm happy for the discussion to be moved.

@jsirois
Copy link
Contributor Author

jsirois commented Jan 6, 2021

Let me tighten up the goal:

As a non-interactive Pip user I'd like to execute it in a subprocess and see no output from it unless there is an error. If there is an error I'd like to display all relevant output.

Given that, with most tools I can run the tool with stdout suppressed and stderr either captured and re-displayed verbatim on error, or with stderr just freely flowing to the termnial if one is present. With Pip however, I cannot since not all the useful error information goes to stderr in the case of dependency conflicts. In that case, some of the error information goes to stderr, some to stdout.

.... and combining stdout and stderr (2>&1) introduces a wealth of irrelevant information to the error. So, clearly - that's an option if I want to swallow the irrelevant information in my output too.

So, as I see it, I have 4 options, 2 involve modifying Pip, 2 I can implement on my own outside of Pip:

  1. Include all relevant conflict error info on stderr via the current logging setup in Pip. That's what I've done in this PR and the side-effect for an interactive user is the helpful conflict error information changes from standard terminal text color to red.
  2. Change the Pip logging configuration to send all logging output to stderr.
  3. Change my non-interactive use of Pip to "2>&1" and swallow the fact that I'm going to show information I don't really want to show when an error occurs (download progress bars, etc).
  4. When using the 2020-resolver, add a --log argument so that - on error - I can grep the log to find all the useful error information and skip all the irrelevant bits.

I implemented 4 over in Pex (pex-tool/pex#1162) and was hoping to implement 1 or 2 over here in Pip so I could undo my workaround in Pex and generally make Pip more non-interactive use friendly.

@pfmoore
Copy link
Member

pfmoore commented Jan 6, 2021

With (1), why not modify the PR just a bit to use a new logging call that outputs to stderr without being in red? I'm not clear why that option's such a problem for you. As far as I can see, warning goes to stderr in yellow, and it wouldn't be impossible to add a new stderr_info level that comes between info and warning and is uncoloured but goes to stderr.

But to be frank, I'm at a loss how this got so out of hand. The only pushback you've had on the original PR was "please don't make all of that information red" and suddenly we're talking about reorganising all of pip's diagnostics. That seems like an over-reaction 🙁

@jsirois
Copy link
Contributor Author

jsirois commented Jan 6, 2021

I'm not clear why that option's such a problem for you.

Thanks, I had 0 clues that was an option! I must have misread feedback. I'll do that.

@pfmoore
Copy link
Member

pfmoore commented Jan 6, 2021

Oh cool! Glad it was just a misunderstanding 🙂

@jsirois
Copy link
Contributor Author

jsirois commented Jan 6, 2021

But to be frank, I'm at a loss how this got so out of hand. The only pushback you've had on the original PR was "please don't make all of that information red"

Well, to be honest, I assumed adding a new non-standard log-level would be a non-starter. Python has standard log levels and fwict ~all projects stick to those. As a new contributor, I tend to assume I can't change infra like that. Not to mention it opens up a whole can of worms - when do folks now use this new level, etc. There is no doc I can see that gives devs guidance on this and so, like I said, seemed like a big can of worms.

@pfmoore
Copy link
Member

pfmoore commented Jan 6, 2021

🤷 it's not ideal, but I'm not against considering it. I'd also be open (personally) to seeing how warning (yellow) looks, or making warning uncoloured. It's only a cosmetic detail, after all.

But equally, I'm still unclear how much benefit you'll get from the change anyway - there's still a load of junk on stderr that you'll have to parse, and I'm not sure how comprehensible either of stdout or stderr is in isolation. So I find it hard to really understand the benefit here. (Which means that if the cost is high, I don't think the change is justified - the problem with the "wall of red" is precisely that the UX cost is high, and UX cost is more significant than internal costs, but they are still important...)

Sorry, that probably feels like I'm being ambivalent again (because I am 🙂)

@jsirois
Copy link
Contributor Author

jsirois commented Jan 6, 2021

Yeah, my take is Pip is interactive focused - which is of course totally fine. As such most approaches here to support non-interactive use are pretty hacky. Ideally, presentation and content would be orthogonal, I.E.: logging a critical would not dictate color and all relevant information could be included in a critical log entry. To get that though would require something like log.log(Level.CRITICAL, red(....) + white(...) + red(...)). I'll think about this all for a while and either close this PR and the associated issue if no viable path seems available or else close this PR and open a new issue (and maybe PR) going down a viable path.

Thanks for your feedback.

@pradyunsg
Copy link
Member

FWIW, I'm on board for completely rewriting pip's UI, if anyone else is interested in that. 🙃

See #8453 and #4649.

@jsirois
Copy link
Contributor Author

jsirois commented Jan 20, 2021

It seems there is no viable path forward here. Completely rewriting Pip's UI is beyond the scope of work I can commit to and the hack of adding an info_stderr log level to support this one split critical_stderr, info_stderr, critical_stderr case to fully output a resolve failure message is something I wouldn't want if I were a Pip maintainer.

I'll close out the associated issue as well. Thanks for the feedback again.

@jsirois jsirois closed this Jan 20, 2021
@jsirois jsirois deleted the issues/9420 branch January 20, 2021 22:09
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include all resolution conflict fix info in error messages or on stderr.
5 participants