(see thread at http://mail.python.org/pipermail/python-ideas/2012-April/014878.html)

This is a patch to add sys.implementation to Python (with doc addition).  The main motivation is to have an explicit place to look for the name and version of the implementation for generating the import cache tag (a la PEP 3147).

If it can contain a variable number of fields, I think it should be a dict rather than a tuple.

*version* is a version tuple, in the format of :data:`sys.version_info`,
   which represents the version of the Python implementation, **not** the
   version of the Python specification it implements.

If that version number is specific to each VM, then I’m not sure we should mandate a specific format.

@antoine - I wondered about that.  In the end I got something up to start with.

The list of fields in sys.implementation may change over time, unlike sys.version_info, et al.  However, during interpreter execution, I would expect that neither that list nor the contents would change.  The variability would only be between implementations and between versions of those implementations.

A dict would imply to me that it might vary during interpreter execution.  An immutable type makes it clear to me that it won't be changing.  I'm fine with a dict, though, if you think that's better.  Perhaps a dictproxy?

@Éric - that's a good point.  I considered it for a little bit, but went with the quick and easy think to get it rolling.

There is a real benefit to mandating an API sys.implementation.version.  importlib would use that version to calculate the tag to use for cached modules.  Without a specified/uniform data structure, that job is trickier.

Having an explicit sys.implementation.cache_tag field would solve that, and the importlib code would check for that field first.  However, I didn't want to start off with that as a "required" field, considering that only CPython would take advantage of module caches (as far as I know).

I'm going to write a PEP for this, after all.  I want to make sure that it's easy to access, in one place, these points that are coming up and their resolutions.

If the main motivation for this is that importlib can use it, I fail to see the point to make it cross-implementation. Other implementations of Python may not use byte code files at all, or use different byte code syntaxes, or not use the marshal module to load byte code. So the part of importlib that deals with cached .pyc files will be specific to CPython, anyway - why make it a cross-implementation attribute?

Thanks for bringing this up, Martin.

sys.implementation is about having an implementation-specific location (hence sys) to consolidate explicit values concerning the implementation.  It's partly about clarifying the run-time distinction between Python-the-language and Python-the-implementation.

'name' and 'version' are the initial fields in sys.implementation because they are the most obvious choices, and a good starting point.  If sys.implementation were available now we'd use it in importlib.  That's what has motivated me to pursue this.

You are correct that we can hard-code "cpython" in the bootstrap code to generate the cache tag.  And perhaps that would be a better use of time.  We can't just use platform.python_implementation(), however, since the frozen importlib can only use builtin modules.

I expect you're right that some of the importlib code (particularly w.r.t. cached modules) is CPython-specific.  However, I'm sure Brett has tried to minimize that subset of the module.  Maybe he knows if the other implementations have plans for using importlib or how it affects them.  If not, I'll ask around.

Regardless, importlib is not the only motivator here.  Right now platform.python_implementation() has to guess the implementation name.  Having the different implementations specify the name explicitly in the sys module would be an improvement without a lot of work.  Nick Coghlan has indicated that it would be useful for the test suite.

Ultimately, sys.implementation would become the source for information about a specific Python implementation.  Others could expound the point better, but as time goes by this will be more important.  This current effort would get the ball rolling.  Consequent to the concerns so far, I'll try to get a PEP out in the next couple days.

So I'm w/ Antoine that a dict would be better so that the other VMs can add whatever they want into that object (e.g. Jython could specify what JVM they are running against) without causing confusion as to what some index means to each VM. The mutability of it is not important IMO.

As for the other implementations using importlib, they all plan to with (hopefully) no direct modification, but whether they support bytecode I don't know (I think Jython has talked about it, but who knows).

And pertaining to whether this is useful outside of importlib, I suspect it is. We already have 'jython' as an "OS" type to work around some differences. Adding this attribute would more clearly delineate when another VM is being used that might require working around some difference without overloading os.name (e.g. PyPy implementing something differently in their 1.7 release of Python 2.7 but is subsequently fixed in their 1.8 release).

When I added sys.subversion, people requested that it shall contain the CPython string. When sys._mercurial was introduced, it copied that. So there are plenty of ways already to figure out that it is CPython which you are using.

An updated patch using a dict.  (FYI, I have the PEP up on python-ideas.)

FYI, the attached patches don't reflect the latest PEP.  I'll get an updated patch up in the next day or two.

I've just attached 4 patches, one for each of the likeliest implementations.  All 4 ran the test suite successfully.

In my mind, the "simple namespace" one is the best fit.  However, it involves adding a new built-in type (though a private one).  If that is too controversial or too much new stuff to iron out for 3.3, I'd be okay with any of the others.  In reality I'd like to see that new namespace type added to Python, but that's a proposition I'll take up elsewhere.  :)

I updated the patches so that (hopefully) the review link shows up.  I've also pulled out the doc/test diff into its own patch, since it was the same for all of them.

One small test that I think is missing is a test that sys.implementation.version and sys.implementation.hexversion are equal (modulo format differences).

I'm not a fan of using a module, and less of a fan of structseq, so I think I'll discount those two.  I'll play with namespace and type next.

I'm inclined to go with the as_simple_namespace patch.  As you say, the pro are that this is a much better fit for this use case, while the con is that this does kind of sneak in a new type.  Given that the type is not exposed in the API, that doesn't bother me.  One other possibility would be to move all that code to sysmodule.c as static functions, and then it definitely won't be available outside sys.  

OTOH, I do think this could eventually be a useful type to expose, however the current behavior of its repr filtering out _names would have to be removed in that case.  However, that's a useful filter for sys.implementation and it doesn't bother me, since you can always repr sys.implementation.__dict__ to get the whole thing.

On balance, my recommendation would be to keep it the way you have it, but perhaps mention this on python-dev, and watch the technicolor bikeshed go psychedelic.

A few other comments as we discussed in irc:

dir(sys.implementation) should return something useful, not traceback

There are a few PEP 7 violations, e.g. brace placements that should be fixed

I was looking at _namespace_init() and a few things bothered me.  I thought this might be superfluous and that you'd be able to just inline the PyDict_Update() calls, but now I'm not so sure.  AFAICT, Python's documentation is silent on whether *args and *kwds in a tp_init function can be NULL or not, and I definitely see cases where similar checks are made in other types.  So I think with that in mind, you must assume they *can* be NULL.  But then I'm not sure the assertions are useful or correct.  Take this scenario:

namespace_init() gets args==NULL.  Your assertion doesn't trigger, but PyObject_Size(NULL) sets an exception and returns -1.  Your conditional doesn't check for an error condition in PyObject_Size() so you'll incorrectly swallow (temporarily) that exception.  At the very least, you need to check for PyObject_Size() < 0.  Don't forget too that those assertions can get compiled away.

I think I'd rather see a PyTuple_Size(args) conditional here for the args parameter.  If it's not a tuple, you'll get an exception set, so check for size < 0.  If size > 0, then you can set the TypeError and return -1 explicitly.

Similarly, just call PyDict_Update(kwds) and return its value.  If kwds is neither a dict nor has a .keys() method (including if its NULL), an exception will be set so everything should work correctly (see _PyObject_CallMethodId() in abstract.c, which is what PyDict_Update() boils down to).

At least, I'm nearly certain that's safe :)

Moving on...

namespace_repr() also has some PEP 7 violations (brace position, extra blank lines).  Please fix these.

Is it ever possible to get into namespace_repr() with ns->ns_dict being NULL?  I think it's impossible.  You might rewrite the if (d == NULL) check with an assertion.

I think you're leaking the PyList_New(0) that you put in `pairs` after you PyUnicode_Join() it.  PyUnicode_Join() doesn't decref its second argument and your losing the original referenced object when you assign `pairs` to its result.

I find the mix of inline error returns and `goto error` handling in namespace_repr() somewhat confusing.  I wonder if it would be more readable (and thus auditable) if there was consistent use of `goto error` with more XDECREFs?  If you feel like it, please try that out.

That's it for now.  Please submit another patch for the as_simple_namespace case and I'll take another look.  Also, if you send a message to python-dev about the introduction of the namespace object, I'll follow up with my support of that option.

Thanks, this is looking great!

History with dictproxy means I'm also OK with "new type by stealth".
Perhaps add some tests to check "type(sys.implementation)()" does something
sane?

Thanks for taking the time for the review, Barry.  Again, sorry I broke the review link.  It shouldn't be a problem any more.

> Barry A. Warsaw <barry@python.org> added the comment:
>
> I'm inclined to go with the as_simple_namespace patch.  As you say,
> the pro are that this is a much better fit for this use case, while
> the con is that this does kind of sneak in a new type.  Given that
> the type is not exposed in the API, that doesn't bother me.  One
> other possibility would be to move all that code to sysmodule.c as
> static functions, and then it definitely won't be available outside
> sys.
>
> OTOH, I do think this could eventually be a useful type to expose,
> however the current behavior of its repr filtering out _names would
> have to be removed in that case.  However, that's a useful filter for
> sys.implementation and it doesn't bother me, since you can always
> repr sys.implementation.__dict__ to get the whole thing.
>
> On balance, my recommendation would be to keep it the way you have
> it, but perhaps mention this on python-dev, and watch the technicolor
> bikeshed go psychedelic.

:)  I'll bring it up on python-dev.

>
> A few other comments as we discussed in irc:
>
> dir(sys.implementation) should return something useful, not traceback

Not sure what I was doing to block the lookup from object, but it's working now.

So...Fixed.

>
> There are a few PEP 7 violations, e.g. brace placements that should
> be fixed

Done.

>
> I was looking at _namespace_init() and a few things bothered me.  I
> thought this might be superfluous and that you'd be able to just
> inline the PyDict_Update() calls, but now I'm not so sure.  AFAICT,
> Python's documentation is silent on whether *args and *kwds in a
> tp_init function can be NULL or not, and I definitely see cases where
> similar checks are made in other types.  So I think with that in
> mind, you must assume they *can* be NULL.  But then I'm not sure the
> assertions are useful or correct.

Yeah, they're mostly an artifact of copying code.  However, I'm glad they triggered your explanation. ;)

> Take this scenario:
>
> namespace_init() gets args==NULL.  Your assertion doesn't trigger,
> but PyObject_Size(NULL) sets an exception and returns -1.  Your
> conditional doesn't check for an error condition in PyObject_Size()
> so you'll incorrectly swallow (temporarily) that exception.  At the
> very least, you need to check for PyObject_Size() < 0.  Don't forget
> too that those assertions can get compiled away.
>
> I think I'd rather see a PyTuple_Size(args) conditional here for the
> args parameter.  If it's not a tuple, you'll get an exception set, so
> check for size < 0.  If size > 0, then you can set the TypeError and
> return -1 explicitly.

Pretty sure I understand.  Let me know if my new patch says otherwise.  <wink>

So...Done.

>
> Similarly, just call PyDict_Update(kwds) and return its value.  If
> kwds is neither a dict nor has a .keys() method (including if its
> NULL), an exception will be set so everything should work correctly
> (see _PyObject_CallMethodId() in abstract.c, which is what
> PyDict_Update() boils down to).

Done.  This was a case of prematurely optimizing.

>
> At least, I'm nearly certain that's safe :)
>
> Moving on...
>
> namespace_repr() also has some PEP 7 violations (brace position,
> extra blank lines).  Please fix these.

Done.

>
> Is it ever possible to get into namespace_repr() with ns->ns_dict
> being NULL?  I think it's impossible.  You might rewrite the if
> (d == NULL) check with an assertion.

Done.

>
> I think you're leaking the PyList_New(0) that you put in `pairs`
> after you PyUnicode_Join() it.  PyUnicode_Join() doesn't decref its
> second argument and your losing the original referenced object when
> you assign `pairs` to its result.

Good Catch.  Fixed.

>
> I find the mix of inline error returns and `goto error` handling in
> namespace_repr() somewhat confusing.  I wonder if it would be more
> readable (and thus auditable) if there was consistent use of `goto
> error` with more XDECREFs?  If you feel like it, please try that out.

I've given it a go.  :)

>
> That's it for now.  Please submit another patch for the
> as_simple_namespace case and I'll take another look.  Also, if you
> send a message to python-dev about the introduction of the namespace
> object, I'll follow up with my support of that option.
>
> Thanks, this is looking great!

Thanks again for all your support through this process!

> History with dictproxy means I'm also OK with "new type by stealth".
> Perhaps add some tests to check "type(sys.implementation)()" does
> something sane?

Test added.  Here's what happens:

  >>> cls = type(sys.implementation)
  >>> cls()
  namespace()
  >>> cls(x=1, y=2)
  namespace(x=1, y=2)

Though it's not immediately a problem, "vars(cls(x=1, y=2))" returns "{}", while "ns=cls(x=1, y=2); vars(ns)" returns "{'x': 1, 'y': 2}"!

Certainly it's a corner case, but it could indicate a more sinister problem.  Regardless, I'll track down the root cause and fix it.  As far as I can tell, this odd behavior does not impact sys.implementation.

@Barry: FWIW, the kwds passed to namespace_init was NULL when no keyword arguments were used.  Easy enough to handle though.

This patch incorporates types.SimpleNamespace and makes the repr show all attributes.  Tests and docs for types.SimpleNamespace are also included.

The patch skips 3 tests (which I added) that are failing.  One has a segfault due to a recursive repr (where I had a hunch that something funny would happen).  One is a situation that I mentioned earlier in this ticket.  However, the third one was entiredly unexpected.

None of these pathologies impact sys.implementation.  I will be working on fixing them regardless.  If you want to wait until I fix the corner cases, I'm fine with that.  However, the patch should be good to go as far as sys.implementation is concerned.

Guess I should have tested the docs before posting the patch. ;)

Looking great, you're almost there!  I remviewed issue14673_as_simple_namespace_2.diff and issue14673_full.diff.  Reitveld makes it *so* much easier :)

sorry, I should have been more clear.  issue14673_full.diff is not simply a merging of the two previous patches, but rather their merger, plus SimpleNamespace, plus removing the "public" restriction from the repr.  I may have a small tweak or two as well.

On Jun 01, 2012, at 06:53 PM, Eric Snow wrote:

>sorry, I should have been more clear.  issue14673_full.diff is not simply a
>merging of the two previous patches, but rather their merger, plus
>SimpleNamespace, plus removing the "public" restriction from the repr.  I may
>have a small tweak or two as well.

Ah dang.  I'd say just double check both of my sets of comments, disregard the
ones that are no longer relevant and push an update for the _full.diff.  I'll
do one final review and/or just commit it for you.

I'll get a new patch up tonight.

Applied all but one of the recommended changes.  Thanks, Barry!

this time against TIP.

I've ironed out all 3 of my new tests that were still failing.

Some remarks:

- From the docs, I could not understand the difference between sys.implementation.version and sys.version_info.  When can they differ?

- _PyNamespace_New should be a public API function.  From Python code, SimpleNamespace is public.

- I agree that _PyNamespace_Type could stay private (it's an implementation detail), in this case PyAPI_DATA is not necessary.

- SimpleNamespace should support weak references.

> - From the docs, I could not understand the difference between
> sys.implementation.version and sys.version_info.  When can they differ?

I'll make an update.  As an example, PyPy is at version 1.8 which implements the Python 2.7 language.  In that case sys.version_info would be (2, 7, 3, 'final', 0) while sys.implementation.version might be (1, 8, 0, 'final', 0).

One confusing part is that for CPython they are exactly the same.  I think there's room for improvement with the versioning of the language specification itself, but one thing at a time!  :)

> - _PyNamespace_New should be a public API function.  From Python code,
> SimpleNamespace is public.
> - I agree that _PyNamespace_Type could stay private (it's an
> implementation detail), in this case PyAPI_DATA is not necessary.

Agreed, though I'd like to see sys.implementation go in first and take action on the PyNamespace type/API in a separate issue.

> - SimpleNamespace should support weak references.

I'll admit that I haven't used weakrefs a ton and am not familiar with the use cases.  However, the fact that most of the builtin types do not support weak references gives me pause to simply adding it.

I'd like more discussion on this, but in a separate issue so that it doesn't hold up sys.implementation.

On Jun 02, 2012, at 11:33 PM, Eric Snow wrote:

>Added file: http://bugs.python.org/file25804/issue14673_full_4.diff

Hi Eric.  I'm ready to do a final review and merge this in, but I just want to
be sure I'm looking at the right file.  Is full_4.diff the most up-to-date
patch, and is it complete (i.e. contains all code, docs, and tests)?

On Jun 02, 2012, at 08:03 AM, Amaury Forgeot d'Arc wrote:

>- _PyNamespace_New should be a public API function.  From Python code,
>- SimpleNamespace is public.

This is a separate discussion.  I'm not opposed, but I don't think this should
be addressed in this patch.

>- SimpleNamespace should support weak references.

Again, this could be addressed separately.

> Is full_4.diff the most up-to-date
> patch, and is it complete (i.e. contains all code, docs, and tests)?

Yep.  :)

New changeset 9c445f4695c1 by Barry Warsaw in branch 'default':
Eric Snow's implementation of PEP 421.
http://hg.python.org/cpython/rev/9c445f4695c1

I'm resolving this as Fixed since I've just committed the code to 3.3, but I'm also leaving its status open and assigning it to mvl for verification of the Windows build.  Martin, if you'd rather not do that, please unassign it from yourself.  I've also added Brian to the nosy list.

The Windows buildbots were failing compilation.

I've added Object/namespaceobject.c and Include/namespaceobject.h to PCbuild/pythoncore.vcxproj in changeset ee7cd7d51ed6.

I think Richard fixed it already, thanks.

presumably PEP 421 can be marked as final now?

On Jun 04, 2012, at 09:39 PM, Eric Snow wrote:

>presumably PEP 421 can be marked as final now?

Done.