Opened 9 years ago

Closed 7 years ago

#16064 closed enhancement (fixed)

Python 3 preparation: Handle basestring (Py2) vs. str (Py3)

Reported by: Wilfried Luebbe Owned by:
Priority: major Milestone: sage-6.8
Component: distribution Keywords: python3
Cc: Merged in:
Authors: André Apitzsch Reviewers: Ralf Stephan, Wilfried Luebbe
Report Upstream: N/A Work issues:
Branch: 7e60f88 (Commits, GitHub, GitLab) Commit: 7e60f88fba8805110e89fa2f7093d61a2d78542d
Dependencies: #18492 Stopgaps:

Status badges

Description

From the Python 2 documentation basestring():

This abstract type is the superclass for str and unicode. It cannot be called or instantiated, but it can be used to test whether an object is an instance of str or unicode. isinstance(obj, basestring) is equivalent to isinstance(obj, (str, unicode)). New in version 2.3. This is not available in Python 3.

The tool 2to3 changes basestring into str (the only string type in Py3).

There are 27 effected modules.

This ticket is tracked as a dependency of meta-ticket ticket:16052.

Change History (31)

comment:1 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:2 Changed 9 years ago by aapitzsch

Authors: André Apitzsch
Branch: u/aapitzsch/ticket/16064
Commit: e1252a45810b1dd857e978336f504843b412bf4d
Status: newneeds_review

New commits:

e1252a4replace basestring by str

comment:3 Changed 9 years ago by Ralf Stephan

Status: needs_reviewpositive_review

Eyeballed and patchbotted ok. Just a naive question: wouldn't the ... str = basestring snippet be needed in every affected file?

comment:4 Changed 9 years ago by Ralf Stephan

Reviewers: Ralf Stephan

comment:5 Changed 9 years ago by Wilfried Luebbe

Status: positive_reviewneeds_work

For unicode strings in Python 2.7 the change will not give the same result as before. See

Python 2.7.6 (default, Mar 22 2014, 22:59:56) 
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> s ='a str'
>>> su = u'a u-str'
>>> isinstance(s, basestring)
True
>>> isinstance(su, basestring)
True
>>> isinstance(s, str)
True
>>> isinstance(su, str)
False
>>> 

As we want a single Python 2 / 3 code base I think this is not the right solution.

comment:6 Changed 9 years ago by git

Commit: e1252a45810b1dd857e978336f504843b412bf4d7fc09f93ebc4b0a662440b79b9d734b1e43d0420

Branch pushed to git repo; I updated commit sha1. New commits:

f4ce4d1replace basestring by str if python version >= 3
7fc09f9Merge remote-tracking branch 'origin/develop' into py3_basestring

comment:7 Changed 9 years ago by aapitzsch

Status: needs_workneeds_review

comment:8 Changed 8 years ago by git

Commit: 7fc09f93ebc4b0a662440b79b9d734b1e43d042063eaffd5133fc3afb87ac12140aa56f800c12abd

Branch pushed to git repo; I updated commit sha1. New commits:

63eaffdMerge remote-tracking branch 'origin/develop' into py3_basestring

comment:9 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:10 Changed 8 years ago by git

Commit: 63eaffd5133fc3afb87ac12140aa56f800c12abd3df268bee99266bfb2d75c3d22edb653089b9928

Branch pushed to git repo; I updated commit sha1. New commits:

3df268bMerge remote-tracking branch 'origin/develop' into py3_basestring

comment:11 Changed 8 years ago by Jeroen Demeyer

I have a general question about Python3 tickets: would it not be better to simply use the 2to3 tool at build time (i.e. when doing sage -b)? This would fix not only this ticket, but also many others and there would be no need to uglify the code like in this patch.

comment:12 in reply to:  11 ; Changed 8 years ago by aapitzsch

You are right using 2to3 would be the better way, but the tool doesn't touch doctests and it will not fix all compatibility issues.

IMO we should change as much as possible (if it is not ugly) upstream to reduce work for 2to3. And in the long run we have to change it anyway. Changes like #16442, #16529 and even #16071 (if we leave the izip part out) should be fine.

Let's skip this ticket for the moment and maybe replaced it by using 2to3 at build time.

comment:13 Changed 8 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

Cython files should support basestring out of the box in Python 3. So no changes are needed to .pyx files.

comment:14 Changed 8 years ago by git

Commit: 3df268bee99266bfb2d75c3d22edb653089b9928bc70272c0df69ebee3b8f080866a61ef900e2f37

Branch pushed to git repo; I updated commit sha1. New commits:

bc70272revert basestring changes in *.pyx files

comment:15 Changed 8 years ago by aapitzsch

Status: needs_workneeds_review

comment:16 in reply to:  12 Changed 8 years ago by Marc Mezzarobba

Milestone: sage-6.4sage-pending
Status: needs_reviewneeds_work

You wrote:

Replying to aapitzsch:

Let's skip this ticket for the moment and maybe replaced it by using 2to3 at build time.

but later set the ticket to needs_review... So I don't understand, what is the status of this ticket? I'm changing both the milestone to pending and the status to needs_work (as there are merge conflicts anyway), please feel free to revert one or both if appropriate.

comment:17 Changed 8 years ago by Jeroen Demeyer

Milestone: sage-pendingsage-6.8

See #18492 for a much better approach using six.

comment:18 Changed 8 years ago by aapitzsch

Branch: u/aapitzsch/ticket/16064u/aapitzsch/ticket/16064_2
Commit: bc70272c0df69ebee3b8f080866a61ef900e2f37
Status: needs_workneeds_review

Made use of six.

I haven't added six as a dependency because that's done in #18492.

comment:19 Changed 8 years ago by aapitzsch

Branch: u/aapitzsch/ticket/16064_2u/aapitzsch/16064_2
Commit: 002892eb3e5ece1d1e1c440831cf03521c36ea66

comment:20 Changed 8 years ago by Jeroen Demeyer

Dependencies: #18492

comment:21 Changed 7 years ago by Wilfried Luebbe

Status: needs_reviewneeds_work

The patch looks fine. Testing is OK.

But in sage-6.8.beta3 there are still basestrings in sage/quivers/algebra.py line 277 and in sage/quivers/path_semigroup.py lines 340 / 348. The are NOT present sage-6.8.beta0 on which the patch is based. I haven't found out why.

Can you base the patch on sage-6.8.beta3 and change the 3 remaining basestrings. Then I would set the ticket to positive review.

comment:22 Changed 7 years ago by git

Commit: 002892eb3e5ece1d1e1c440831cf03521c36ea66b16df45af9855bad1a2687db25d57e8fa881a8cf

Branch pushed to git repo; I updated commit sha1. New commits:

eaa8dc7Merge remote-tracking branch 'origin/develop' into py3_basestring_six
b16df45replace remaining basestring by six.string_types

comment:23 Changed 7 years ago by aapitzsch

Status: needs_workneeds_review

comment:24 Changed 7 years ago by Wilfried Luebbe

Right at the beginning of test --all --long (after a fresh make!) I get LOTS of errors like

File "src/sage/combinat/root_system/non_symmetric_macdonald_polynomials.py", line 62, in sage.combinat.root_system.non_symmetric_macdonald_polynomials.NonSymmetricMacdonaldPolynomials
Failed example:
    E = NonSymmetricMacdonaldPolynomials(["A",2,1])
Exception raised:
    Traceback (most recent call last):
      File "/home/wluebbe/sage-6.8.beta3/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/wluebbe/sage-6.8.beta3/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.root_system.non_symmetric_macdonald_polynomials.NonSymmetricMacdonaldPolynomials[0]>", line 1, in <module>
        E = NonSymmetricMacdonaldPolynomials(["A",Integer(2),Integer(1)])
      File "sage/misc/lazy_import.pyx", line 383, in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3453)
        return self._get_object()(*args, **kwds)
      File "sage/misc/classcall_metaclass.pyx", line 326, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1204)
        return cls.classcall(cls, *args, **kwds)
      File "/home/wluebbe/sage-6.8.beta3/local/lib/python2.7/site-packages/sage/combinat/root_system/non_symmetric_macdonald_polynomials.py", line 1208, in __classcall__
        q = K(q)
      File "sage/structure/parent.pyx", line 1095, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9523)
        return mor._call_(x)
      File "sage/structure/coerce_maps.pyx", line 109, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4402)
        raise
      File "sage/structure/coerce_maps.pyx", line 104, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4309)
        return C._element_constructor(x)
      File "/home/wluebbe/sage-6.8.beta3/local/lib/python2.7/site-packages/sage/rings/fraction_field.py", line 563, in _element_constructor_
        x = sage_eval(x, self.gens_dict_recursive())
      File "/home/wluebbe/sage-6.8.beta3/local/lib/python2.7/site-packages/sage/misc/sage_eval.py", line 181, in sage_eval
        if not isinstance(source, six.string_types):
    AttributeError: 'module' object has no attribute 'string_types'

Apparently the module six has not yet the attribute string_types. This seems to be the symptom of a circular import. But why after this last, small change?

comment:25 Changed 7 years ago by Wilfried Luebbe

Apparently both fraction.field.py and sage-eval.py (see the last 2 listed files) contain import six. And at the point where the error occurs the import process for module six (started in fraction.field.py) is not yet complete.

comment:26 Changed 7 years ago by git

Commit: b16df45af9855bad1a2687db25d57e8fa881a8cf7e60f88fba8805110e89fa2f7093d61a2d78542d

Branch pushed to git repo; I updated commit sha1. New commits:

7e60f88prevent relative import of six

comment:27 Changed 7 years ago by aapitzsch

There is a new six.py in misc which is relative imported instead of the expected six module.

comment:28 Changed 7 years ago by Wilfried Luebbe

Reviewers: Ralf StephanWilfried Luebbe
Status: needs_reviewpositive_review

I did not think of a new six.py module in src/sage/misc!

All test passed. Looks good.

comment:29 Changed 7 years ago by Ralf Stephan

Reviewers: Wilfried LuebbeRalf Stephan, Wilfried Luebbe

You were not satisfied with my part of the review, sir?

comment:30 in reply to:  29 Changed 7 years ago by Wilfried Luebbe

Replying to rws:

You were not satisfied with my part of the review, sir?

Sorry about that.

comment:31 Changed 7 years ago by Volker Braun

Branch: u/aapitzsch/16064_27e60f88fba8805110e89fa2f7093d61a2d78542d
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.