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

Warnings in backends as suppressed by frontends #558

Open
jaraco opened this issue Nov 29, 2021 · 12 comments
Open

Warnings in backends as suppressed by frontends #558

jaraco opened this issue Nov 29, 2021 · 12 comments

Comments

@jaraco
Copy link
Member

jaraco commented Nov 29, 2021

Problem description

In pypa/setuptools#2893, I learned that pip's handling of Setuptools and more generally the subprocess mechanism for frontends handling of backends means that warnings are absorbed by the frontend. A backend has primarily one way to signal a problem (fail with an exit code).

By default, users don't see any warnings issued by a backend and it's difficult for users to increase the visibility. It's possible to turn warnings into errors with -We or similar, but that unfortunately triggers other errors. Users could pass -vv or similar to the frontend, but then they get a wall of text with any warnings buried in the output.

Better would be if there was a mechanism for backends to log warnings in a way that they could be emitted by default.

Perhaps the protocol could allow for backends to emit output at different levels, similar to log levels, and then structure it such that the frontend could give users control over visibility and use their magic to make the output more accessible.

@layday
Copy link
Member

layday commented Nov 29, 2021

Making warnings visible in pip is not an effective means of alerting package authors about deprecations and upcoming removals. These tend to go unnoticed in what is usually an extremely long stream of text (cf. warnings being converted to errors in test suites). It is also important to recognise that the primary target audience of pip is not package authors. Printing out warnings would just be adding noise to the majority of its users' consoles.

A necessary precondition, I think, for moving this issue forward is for package authors to use build or another bespoke build tool, which does not hide stderr and which would have the capacity to innovate. As you say raising for all warnings is not practicable, but backend authors could agree on a warning category that build tools should display prominently or raise for by default. We could codify such a category in PEP 517 or another mini-PEP.

@layday
Copy link
Member

layday commented Nov 29, 2021

Paging @pypa/build-committers and @pypa/pip-committers for their input.

@pfmoore
Copy link
Member

pfmoore commented Nov 29, 2021

The reason for this is that the only means for backends to pass data to the frontend is via stdout/stderr. And the inconsistent use of stdout vs stderr (by tools run by the backend, for example, compilers) means that even treating stdout and stderr independently is risky.

So the communication is ultimately a single stream of (unstructured) data. Pip suppresses this because it's typically long, and unhelpful/distracting when part of a sequence of many installs. As you note, it's available via -v if the user cares.

If we want a richer way of communicating messages between the backend and frontends (whether that's for warnings, or anything else), I agree it would ultimately need to be an update to the spec. Unless it's necessary for it to be "real time" (displayed to the user as the messages are generated) I would suggest the best approach would be via return values from the hooks (or maybe a new hook like get_extended_output) rather than trying to make the output stream parseable (thanks to mishandling of codepages by some compilers, there are cases where the output stream isn't even valid Unicode!)

@layday
Copy link
Member

layday commented Nov 29, 2021

The reason for this is that the only means for backends to pass data to the frontend is via stdout/stderr. [...]

But the PEP 517 interface is not standard I/O. Frontends use a "negotiator", which tends to be the pep517 library, which invokes hooks in a subprocess; the warning filtering could simply be done in pep517, within the subprocess. I don't think anything fancier is needed here than an optional Pep517BuildWarning class (bikeshedding can follow at a later stage), that if importable from the backend, and if emitted, could be converted to an error or handled in whatever special way.

@pfmoore
Copy link
Member

pfmoore commented Nov 29, 2021

There's no requirement for frontends to use a library. See https://www.python.org/dev/peps/pep-0517/#build-environment. And certainly there's no requirement to use the specific library pep517.

So you'd have to describe the interface in library-neutral terms. And remember the provision in the PEP, "Frontends should call each hook in a fresh subprocess" - so however warning information is communicated, it must be possible to do so across a process boundary.

But TBH, apart from how this fits into the standards process, I don't actually care about the implementation technique used. I think it's reasonable (although rare) to want to be able to raise warnings in the backend, and having a way to do so would be good. But Discourse might be a better place to discuss implementation than here.

@layday
Copy link
Member

layday commented Nov 29, 2021

There's no requirement for frontends to use a library. See https://www.python.org/dev/peps/pep-0517/#build-environment. And certainly there's no requirement to use the specific library pep517.

That's the point that I was making, that handling these warnings right now would only be inhibited by pep517's lack of support. Put another way, <your frontend> has only got to scour stdio if <your subprocess runner> writes warnings to stdio. How it might get warnings across the process boundary is no different than the rest of PEP 517's output. There's no reason that the subprocess's stdout should contain anything but messages for the frontend, or that all of the backend's output can't be shoehorned into stderr, or that you can't use x messaging technology for IPC.

@uranusjr
Copy link
Member

uranusjr commented Nov 29, 2021

I wonder if we should amend the interface to add an additional text file for the backend to tell the frontend they want to display something.

@pfmoore
Copy link
Member

pfmoore commented Nov 29, 2021

I'm confused. But it's not that important. I suggest that we avoid speculating until someone proposes a precise mechanism that a PEP 517 backend can use to signal there's a warning, and then discuss how a frontend would capture that warning (it doesn't matter whether the frontend in question is a library like pep517¹, or a build frontend like pip doing direct calls to the backend).

Although actually, I wonder - are you suggesting that backends simply call the standard Python warnings mechanism, and the front end then arranges to catch warnings if it wants to process them? Maybe that would work? I've not used Python's warnings mechanism much, so I'm not sure...

¹ I really wish the library would change its name. It's nearly impossible to be clear when talking about both PEP 517 the standard, and pep517 the library at the same time 🙁

@uranusjr
Copy link
Member

I was just wondering that, since the main problem is backends are using standard streams inconsistently (because how those streams should be used are inherently inconsistent), the solution may be to simply define a stream that can be used consistently between PEP 517 backend and frontend.

@layday
Copy link
Member

layday commented Nov 29, 2021

I'm confused. But it's not that important. I suggest that we avoid speculating until someone proposes a precise mechanism that a PEP 517 backend can use to signal there's a warning, and then discuss how a frontend would capture that warning (it doesn't matter whether the frontend in question is a library like pep517¹, or a build frontend like pip doing direct calls to the backend).

Well, I thought I had 😛 And yes, I was suggesting that the backend simply emits a warning, so that:

def build_wheel(...):
    warnings.warn("option x is deprecated, use y", BuildWarning)

A naive implementation in the frontend would be something like:

import warnings

def build_warning_filter(build_warnings: list[str]):
    try:
        from backend import BuildWarning
    except ImportError:
        yield
    else:
        orig_showwarning = warnings.showwarning

        def showwarning(message, category, *args):
            if category is BuildWarning:
                build_warnings.append((message, category, *args))
            else:
                orig_showwarning(message, category, *args)
        
        warnings.showwarning = showwarning
        yield
        warnings.showwarning = orig_showwarning

def call_some_hook():
    build_warnings: list[str] = []
    with build_warning_filter(build_warnings):
        my_backend.build_wheel(...)
    
    # Convert build_warnings to JSON or whatever you wanna do
    # to pass them to the main process

@pfmoore
Copy link
Member

pfmoore commented Nov 29, 2021

Ah, OK. As I said, I'm not familiar with the warnings infrastructure. Sorry, this is indeed basically what you were suggesting from the start - my bad.

Arguably, this is allowed within the current PEP 517, it's just that existing frontends don't catch or handle warnings. In which case, all we need to do is:

  1. Add a clarification to the spec noting that backends could potentially raise warnings, and frontends should be prepared for that possibility. (It remains up to the front end whether to display such warnings or not, and that's fine IMO).
  2. Submit a PR for pep517 (the library) updating the API to handle warning processing. Backward compatibility needs to be considered, but otherwise there's no technical problem with doing this.
  3. Submit PRs for pip, build and any other tools that use pep517 (the library) to use the new interface.

As far as the spec is concerned, this would be a minor clarification rather than a substantive update. If PEP 517 was hosted on packaging.python.org, it could be just a PR to the document. But currently the master copy of the spec is the PEP itself, so the PEP process applies (and technically, PEPs shouldn't be updated after acceptance). I'd be inclined to say that the clarification should be combined with moving the spec to packaging.python.org (something that's long overdue, IMO), but that would require its own PEP, so maybe a mini-PEP covering the move and the clarification would be the best option.

@layday
Copy link
Member

layday commented Nov 29, 2021

That's a good overview, thanks.

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

No branches or pull requests

4 participants