classification
Title: implement warnings module in C
Type: Stage:
Components: Interpreter Core Versions: Python 2.6
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: brett.cannon, christian.heimes, nnorwitz
Priority: high Keywords: patch

Created on 2007-01-09 06:29 by nnorwitz, last changed 2008-04-12 23:44 by brett.cannon. This issue is now closed.

Files
File name Uploaded Description Edit
c_warnings.diff brett.cannon, 2008-04-01 20:07 Complete patch
Messages (25)
msg51714 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-01-09 06:29
Re-implement the warnings module in C for speed and to reduce start up time.

I don't remember the exact state of this patch.  I'm sure it needs cleanup.  IIRC the only thing missing feature-wise was processing command line arguments.  Though I'm not entirely sure.  It's been a while since I did it.

I think I may have not used as many goto's in the code.  I'm also thinking I didn't like it as the error handling was too complex.  This definitely needs review.  If anyone wants to finish this off, go for it.  I'll probably return to it, but it won't be for a few weeks at the earliest.  It would probably be good to make comments to remind me of what needs to be done.

The new file should be Python/_warnings.c.  I couldn't decide whether to put it under Python/ or Modules/.  It seems some builtin modules are in both places.  Maybe we should determine where the appropriate place is and move them all there.

I couldn't figure out how to get svn to do a diff of a file that wasn't checked in.  I think I filtered out all the unrelated changes.
msg51715 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-01-09 06:30
File Added: _warnings.c
msg56283 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-10-09 05:43
I have uploaded my touched up version of _warnings.c .  I also have a
new diff since the old one did not apply cleanly (test_warnings didn't
get patched nor did PCbuild8).  It also lacked the changes needed to
Modules/config.c .

It is still not complete, though.  Some things are missing from
_warnings.c and test_warnings is completely failing.  But I figured I
should get something uploaded so that other people can help if they want.
msg56288 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-10-09 17:55
I just tried Neal's version of _warnings.c and it has the same failures,
so they are not my fault.  =)
msg56291 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-10-09 19:53
I figured out why the tests are all failing; the C code does not call
back into the Python 'warnings' wrapper.

For instance, warn_explicit in the C code does not ever try to use a
user-provided showwarning() function.  This causes the tests to fail as
they rely on this functionality.

There is also the issue of the filters and once_registry also only be
referenced in the C code and not in the Python code.  So while the list
and dict, respectively, are assigned in warnings.py from _warnings.c,
the C code never checks to see if the attributes in the Python code were
changed.  This is an issue as a lot of times code does::

  warnings.filter = []
  ... code ...
  warnings.filter = original_filter

That will not work with how it is implemented now as the C code only
works off of the object it created and never one that a user may have
provided.

Could a descriptor be written so that when the Python code has the
filter, once_registry, or showwarnings set it actually gets set in the C
code?  That way the C code can continue to be fully independent of the
Python code and not have to import it unless a specific change was
attempted upon 'warnings'?
msg56409 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-10-14 04:55
So the descriptor idea didn't work.

Another idea is to have the C code that relies on attributes on warnings
that are allowed to change have an initial check for warnings, and if
that fails to fall back on C code.  That way the module can still be
completely self-sufficient while still allowing users to change values
from Python code.

For instance, take warnings.filters.  Initially it can be set to
_warnings.filters.  But in the C code, where access to the filters list
is needed, a check is first done to see if the warnings module has been
imported.  If so, check for a filters attribute.  If it exists, replace
the internal list with that one and continue on.  If the module is not
there, however, use the list stored statically in the module without
change.  Same idea of the once registry and a similar idea for
showwarnings().
msg56509 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-10-17 00:35
Attached is a new version of _warnings.c that checks to see if
'warnings' has been imported, and if so, uses the attributes from that
module for onceregistry and 'filters'.  I did it in such a way so that
'warnings' is in no way required nor imported through checking.

If Neal does a code review and OKs the approach then it can also be
implemented for showwarning() which should make testing test_warnings
possible.  =)  Then we can start testing that changes to the module
attributes actually affect things properly.
msg56517 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-10-18 03:53
I think this is good enough for now.  The approach will probably stand
even if the details change.  Go for it!
msg56545 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-10-18 21:41
Regression test suite now passes.  =)  Had to add the support for when
warnings.showwarning() is set and a bug in PyErr_WarnExplicit() where a
NULL value for the module name was not being allowed.

Couple things still left to implement.  One is the second output line in
show_warning().  Don't notice this in unit tests as it imports warnings
and thus uses the Python implementation that works properly.  Also need
to implement warn_explicit().  Lastly, -W arguments need to be handled.

In terms of differing semantics, file paths are different.  Old way did
absolute paths.  It also handled what the file path should be when
executing a warning from the interpreter differently.

In terms of testing, some _warnings-specific tests are needed.  As
mentioned above, the differences between _warnings.show_warning() and
warnings.showwarning() are not being picked up.  This shows how tests
for setting of 'filters', 'onceregistry', and 'showwarning' needs to be
tested.

Assigning to Neal to see if there is anything I missed in terms of todos
and see if he wants to take any of them on.  =)
msg56983 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-10-31 04:22
I think Brett summarized the issues well.  I can't think of anything
else that seems to need doing.
msg57627 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-11-18 23:45
This version of test_warnings has tests for _warnings.c.  It currently
is failing as the second line of output for warnings has not been
implemented yet for _warnings.c.

But all the specified tests in my last comment have now been implemented.
msg57670 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-11-19 22:35
Attached is a diff against the trunk (including _warnings.c) that adds
the second line of output.

That leaves warn_explicit() and handling -W arguments on the TODO list.
 I really don't want to do the -W arguments, so anyone who wants to take
that on, feel free.  =)
msg57767 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-11-22 20:26
Implementing warn_explicit() is going to be troublesome with the new
module_globals argument.  It requires not only to go through the hassle
of looking for a loader and calling get_source(), but it will most
likely require reworking the traceback function introduced to print out
the second line of output (it doesn't look like it directly supports
loaders, but I have not tried it personally).
msg57968 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-11-30 07:57
I see two ways of implementing the fetching of a source code line from
__loader__.get_source().

One is to do it in Python.  We have a function provided that can
suppress the second line of output from a warning and just handle it in
Python code.  That has the requirement that Python code be available to
import, but if you are using Lib/warnings.py instead of
Python/_warnings.c that is pretty much guaranteed.

The other option is to rely on the fact that get_source() is supposed to
use universal newlines.  Then we can find the index of the x and x-1
newlines and print the substring between the two.  That can be done in C
code by checking for the loader, checking for get_source(), calling it,
getting the char buffer, and then just looping through looking for the
needed newlines to find the needed indexes.  Otherwise we can use the
Python API on strings to do what would have been done in pure Python,
but that is a lot of PyObject_Call() usage and seems overly inefficient
if one bothers with coding it in C.  =)
msg58110 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2007-12-03 04:19
The attached patch implements warn_explicit(), all in C.  Tests are
still needed, though (including for showwarning() and formatwarning() to
deal with their new 'line' argument).

And -W still needs to be dealt with.
msg62708 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-02-22 23:36
Patch that has been brought up-to-date with r60968. No new work, though.
msg63510 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-03-13 20:07
Add tests for the 'line' argument to formatwarning() and showwarning().
msg64040 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-03-19 05:14
Attached is what I think is a completely working version of warnings 
implemented in a mixture of C and Python. I am not worrying about 
documenting the new C APIs I had to add since they are pretty specific 
to internal stuff. Probably should add a leading '_', but I'm tired. =)

I think the fleshed out tests do a pretty good job of testing new code. 
Even if this patch gets held up the tests should definitely get 
backported as is reasonable.

Assigning to Neal to review (hopefully soon).
msg64780 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-03-31 10:11
On another note, the warnings module should be made to work with or 
without _warnings. that way IronPython, Jython, and PyPy won't have to re-
implement stuff. This also means that test cases need to be changed to 
test this.
msg64801 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2008-04-01 04:48
I didn't realize this was waiting for me.  You should have just checked
it in, that would have gotten me to review faster. :-)

pythonrun.c:
 * Should PyModule_GetWarningsModule() return a valid pointer?
 * The code below crashes.  Need to XDECREF, not DECREF (or similar).
+        PyObject *warnings_module = PyImport_ImportModule("warnings");
+        if (!warnings_module)
+            PyErr_Clear();
+        Py_DECREF(warnings_module);

Python/_warnings.c:
 * Remove // XXX(nnorwitz): need to parse -W cmd line flags

Include/pythonrun.h
 * init_warnings has the wrong name (not prefixed with _Py).  I'm not
sure it should be exported at all.

test_support/frozen:  did you want the captured_std{out,err} change in
this patch?

Changes to Makefile.pre.in other than adding _warnings.o?

I think this is good enough if it's working.  How about checking it in
after 1) the alpha is released Wed and 2) fixing up the nits?
msg64803 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-04-01 07:56
After all the threats about checking in code that break stuff, I am not 
about to check this in. =)

I will get to the changes when I can and then commit after the alpha.
msg64808 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-04-01 13:14
Neal's issues are addressed in this patch. I also finally filled out 
warnings.h. The only thing that I didn't deal with is Neal's worry of 
exposing _PyWarnings_Init(). It is not explicitly exported anywhere as 
part of the API so I am not sure what he is worrying about.

The attached patch is finished for CPython. I do want to go back and put 
back in the pure Python code that _warnings.c replaces so that 
alternative VMs don't have to implement any part of warnings if they 
don't want to.
msg64820 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-04-01 20:07
Attached should have everything, including a pure Python fallback. As soon 
as the next alpha is out I will apply.
msg64831 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2008-04-02 02:31
On Tue, Apr 1, 2008 at 6:14 AM, Brett Cannon <report@bugs.python.org> wrote:
>
>  Brett Cannon <brett@python.org> added the comment:
>
>  Neal's issues are addressed in this patch. I also finally filled out
>  warnings.h. The only thing that I didn't deal with is Neal's worry of
>  exposing _PyWarnings_Init(). It is not explicitly exported anywhere as
>  part of the API so I am not sure what he is worrying about.

I wasn't so worried about exposing it, I didn't like the name
pollution (we're talking about init_warnings, right?).  I know that we
have other modules that use init*, but those are broken too.  I'm not
sure we should fix those in 2.6, but 3.0 we should.
msg65427 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2008-04-12 23:44
Committed in revision 62303.
History
Date User Action Args
2008-04-12 23:44:28brett.cannonsetstatus: pending -> closed
resolution: accepted
messages: + msg65427
2008-04-02 02:31:16nnorwitzsetmessages: + msg64831
2008-04-01 20:07:30brett.cannonsetfiles: - c_warnings.diff
2008-04-01 20:07:22brett.cannonsetstatus: open -> pending
files: + c_warnings.diff
messages: + msg64820
2008-04-01 13:35:29brett.cannonsetfiles: - c_warnings.diff
2008-04-01 13:14:34brett.cannonsetfiles: + c_warnings.diff
messages: + msg64808
2008-04-01 07:56:51brett.cannonsetassignee: nnorwitz -> brett.cannon
messages: + msg64803
2008-04-01 04:48:32nnorwitzsetmessages: + msg64801
2008-03-31 10:11:07brett.cannonsetpriority: normal -> high
messages: + msg64780
2008-03-19 05:14:46brett.cannonsetfiles: + c_warnings.diff
assignee: brett.cannon -> nnorwitz
messages: + msg64040
2008-03-19 05:08:43brett.cannonsetfiles: - c_warnings.diff
2008-03-19 05:08:39brett.cannonsetfiles: - c_warnings.diff
2008-03-13 20:07:27brett.cannonsetfiles: + c_warnings.diff
messages: + msg63510
2008-02-22 23:37:54brett.cannonsetfiles: - _warnings.diff
2008-02-22 23:37:51brett.cannonsetfiles: - warnings.diff
2008-02-22 23:37:47brett.cannonsetfiles: - test_warnings.py
2008-02-22 23:37:43brett.cannonsetfiles: - _warnings.c
2008-02-22 23:37:39brett.cannonsetfiles: - _warnings.c
2008-02-22 23:37:36brett.cannonsetfiles: - c_warnings.diff
2008-02-22 23:37:33brett.cannonsetfiles: - _warnings.c
2008-02-22 23:37:30brett.cannonsetfiles: - _warnings.c
2008-02-22 23:36:34brett.cannonsetfiles: - c-warnings.diff
2008-02-22 23:36:25brett.cannonsetfiles: + c_warnings.diff
messages: + msg62708
2007-12-03 04:19:03brett.cannonsetfiles: + _warnings.diff
messages: + msg58110
2007-11-30 07:57:06brett.cannonsetmessages: + msg57968
2007-11-22 20:26:55brett.cannonsetmessages: + msg57767
2007-11-19 22:35:58brett.cannonsetfiles: + warnings.diff
messages: + msg57670
2007-11-18 23:45:05brett.cannonsetfiles: + test_warnings.py
messages: + msg57627
2007-11-16 03:24:38brett.cannonsetassignee: nnorwitz -> brett.cannon
2007-11-09 21:46:30christian.heimessetnosy: + christian.heimes
2007-10-31 04:22:02nnorwitzsetmessages: + msg56983
2007-10-18 21:41:27brett.cannonsetfiles: + _warnings.c
assignee: brett.cannon -> nnorwitz
messages: + msg56545
2007-10-18 03:54:02nnorwitzsetassignee: nnorwitz -> brett.cannon
2007-10-18 03:53:34nnorwitzsetmessages: + msg56517
2007-10-17 00:35:28brett.cannonsetfiles: + _warnings.c
assignee: brett.cannon -> nnorwitz
messages: + msg56509
2007-10-14 04:55:59brett.cannonsetmessages: + msg56409
2007-10-09 19:53:18brett.cannonsetmessages: + msg56291
2007-10-09 17:55:15brett.cannonsetmessages: + msg56288
2007-10-09 05:43:48brett.cannonsetfiles: + c_warnings.diff
2007-10-09 05:43:14brett.cannonsetfiles: + _warnings.c
messages: + msg56283
2007-09-30 19:32:01brett.cannonsetassignee: brett.cannon
nosy: + brett.cannon
2007-01-09 06:29:21nnorwitzcreate