Opened 11 months ago

Closed 4 months ago

Last modified 4 months ago

#26592 closed defect (fixed)

py3: combinat/designs: bytes-like object is required, not 'str'

Reported by: slabbe Owned by:
Priority: major Milestone: sage-8.8
Component: python3 Keywords: thursdaysbdx
Cc: dgordon, slabbe, slelievre, vklein, zerline Merged in:
Authors: Samuel Lelièvre Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 1f3afa8 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by slabbe)

sage: print(version())
SageMath version 8.5.beta1, Release Date: 2018-10-27
sage: print(sys.version)
3.6.6 (default, Oct 27 2018, 01:34:38)
[GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)]
sage: C = designs.best_known_covering_design_from_LJCR(7, 3, 2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-a3d9f9e592f1> in <module>()
----> 1 C = designs.best_known_covering_design_from_LJCR(Integer(7), Integer(3), Integer(2))

/opt/s/sage/local/lib/python3.6/site-packages/sage/combinat/designs/covering_design.py in best_known_covering_design_www(v, k, t, verbose)
    505     f.close()
    506
--> 507     if 'covering not in database' in s:   #not found
    508         str = "no (%d,%d,%d) covering design in database\n"%(v,k,t)
    509         raise ValueError(str)

TypeError: a bytes-like object is required, not 'str'

Change History (39)

comment:1 Changed 11 months ago by slelievre

The whole https://math.ccrwest.org was down for a few days. It's back up now.

comment:2 Changed 11 months ago by slelievre

This needs some Python 3 love now though.

Here's what I get with a Python3-based Sage:

sage: print(version())
SageMath version 8.5.beta1, Release Date: 2018-10-27
sage: print(sys.version)
3.6.6 (default, Oct 27 2018, 01:34:38)
[GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)]
sage: C = designs.best_known_covering_design_from_LJCR(7, 3, 2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-a3d9f9e592f1> in <module>()
----> 1 C = designs.best_known_covering_design_from_LJCR(Integer(7), Integer(3), Integer(2))

/opt/s/sage/local/lib/python3.6/site-packages/sage/combinat/designs/covering_design.py in best_known_covering_design_www(v, k, t, verbose)
    505     f.close()
    506
--> 507     if 'covering not in database' in s:   #not found
    508         str = "no (%d,%d,%d) covering design in database\n"%(v,k,t)
    509         raise ValueError(str)

TypeError: a bytes-like object is required, not 'str'

comment:3 Changed 11 months ago by slelievre

Feel free to repurpose the present ticket for that.

comment:4 Changed 10 months ago by slabbe

  • Component changed from doctest coverage to python3
  • Description modified (diff)
  • Summary changed from internet doctest failing in combinat/designs [SSL: UNKNOWN_PROTOCOL] to py3: combinat/designs: bytes-like object is required, not 'str'

Ok, I updated the goal of this ticket as suggested.

comment:5 Changed 9 months ago by chapoton

  • Milestone changed from sage-8.5 to sage-8.7

comment:6 Changed 6 months ago by slabbe

As suggested in https://trac.sagemath.org/ticket/26502#comment:8, the url address apparently changed to:

http://dmgordon.org/cover/

So we need to redo the changes that were made in #26502 in order to fix this issue.

comment:7 Changed 6 months ago by vklein

The problem with dmgordon.org is that the server certificate chain is incomplete:

sage: from six.moves.urllib.request import urlopen
sage: url = "https://dmgordon.org/cover/"
sage: f = urlopen(url)
---------------------------------------------------------------------------
URLError                                  Traceback (most recent call last)
<ipython-input-6-152a0d5393b6> in <module>()
----> 1 f = urlopen(url)

...

/home/vklein/odk/sage/local/lib/python2.7/urllib2.pyc in do_open(self, http_class, req, **http_conn_args)
   1196         except socket.error, err: # XXX what error?
   1197             h.close()
-> 1198             raise URLError(err)
   1199         else:
   1200             try:

URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:726)>

Should we find a way to ignore ssl verification or to contact dmgordon.org owner ?

Last edited 6 months ago by vklein (previous) (diff)

comment:8 Changed 6 months ago by slelievre

  • Cc slelievre added

I just emailed Dan Gordon to report the various issues we are trying to tackle here:

comment:9 Changed 6 months ago by vklein

Thank you Samuel.
For the http vs https part, note that urlopen("http://dmgordon.org/cover/get_cover.php") return the famous HTTP Error 404: Not Found.

Last edited 6 months ago by vklein (previous) (diff)

comment:10 Changed 6 months ago by vklein

  • Keywords thursdaysbdx added

comment:11 Changed 6 months ago by slelievre

My attempt so far is using:

  • src/sage/combinat/designs/covering_design.py

    diff --git a/src/sage/combinat/designs/covering_design.py b/src/sage/combinat/designs/covering_design.py
    index 87dda39064..23beebc2e8 100644
    a b def best_known_covering_design_www(v, k, t, verbose=False): 
    497497
    498498    param = ("?v=%s&k=%s&t=%s"%(v,k,t))
    499499
    500     url = "https://math.ccrwest.org/cover/get_cover.php"+param
     500    url = "http://ljcr.dmgordon.org/cover/show_cover.php" + param
    501501    if verbose:
    502502        print("Looking up the bounds at %s" % url)
    503503    f = urlopen(url)

which returns html instead of plain text, but I guess we could easily de-html-ify it.

comment:12 Changed 6 months ago by vklein

It's feasible but calling get_cover.php will be far cleaner.
Another concern for me is that without https, given that the current code call sage_eval on the result, this code look like an open door to inject any (sage) code you want on a sage user's computer.

Last edited 6 months ago by vklein (previous) (diff)

comment:13 Changed 6 months ago by slelievre

  • Cc dgordon slabbe vklein added

The following changes should suffice once get_cover.php is repaired.

(I think it currently gives an extra initial \n\n<html>\n<body>\n we don't expect.)

  • src/sage/combinat/designs/covering_design.py

    diff --git a/src/sage/combinat/designs/covering_design.py b/src/sage/combinat/designs/covering_design.py
    index 87dda39064..ef7512b704 100644
    a b design from this database, we include extra information: 
    2121REFERENCES:
    2222
    2323.. [1] La Jolla Covering Repository,
    24   https://math.ccrwest.org/cover.html
     24  http://ljcr.dmgordon.org/cover.html
    2525
    2626.. [2] Coverings,
    2727  Daniel Gordon and Douglas Stinson,
    28   https://math.ccrwest.org/gordon/hcd.pdf
     28  https://drive.google.com/file/d/0B6s9_JNV1EjtUFd4MkhBc0hSdVU/view
    2929  from the Handbook of Combinatorial Designs
    3030
    3131AUTHORS:
    class CoveringDesign(SageObject): (this hunk was shorter than expected) 
    452452
    453453def best_known_covering_design_www(v, k, t, verbose=False):
    454454    r"""
    455     Gives the best known `(v,k,t)` covering design, using the database
    456     available at `<https://math.ccrwest.org/cover.html>`_
     455    Return the best known `(v, k, t)` covering design from an online database.
     456
     457    This uses the La Jolla Covering Repository, a database
     458    available at `<http://ljcr.dmgordon.org/cover.html>`_
    457459
    458460    INPUT:
    def best_known_covering_design_www(v, k, t, verbose=False): 
    488490    """
    489491    # import compatible with py2 and py3
    490492    from six.moves.urllib.request import urlopen
    491 
     493    from sage.cpython.string import bytes_to_str
    492494    from sage.misc.sage_eval import sage_eval
    493495
    494496    v = int(v)
    def best_known_covering_design_www(v, k, t, verbose=False): 
    497499
    498500    param = ("?v=%s&k=%s&t=%s"%(v,k,t))
    499501
    500     url = "https://math.ccrwest.org/cover/get_cover.php"+param
     502    url = "http://ljcr.dmgordon.org/cover/get_cover.php" + param
    501503    if verbose:
    502504        print("Looking up the bounds at %s" % url)
    503     f = urlopen(url)
    504     s = f.read()
    505     f.close()
     505    with urlopen(url) as f:
     506        s = bytes_to_str(f.read())
    506507
    507508    if 'covering not in database' in s:   #not found
    508509        str = "no (%d,%d,%d) covering design in database\n"%(v,k,t)

I'll propose a branch if Dan Gordon confirms this about get_cover.php.

comment:14 Changed 6 months ago by slelievre

Dan Gordon was able to get rid of the extra initial \n\n<html>\n<body>\n. We're almost there, but one thing puzzles me about sage -t.

With the following code in src/sage/combinat/designs/covering_design.py

def best_known_covering_design_www(v, k, t, verbose=False):
    r"""
    Return the best known `(v, k, t)` covering design from an online database.

    This uses the La Jolla Covering Repository, a database
    available at `<http://ljcr.dmgordon.org/cover.html>`_

    INPUT:

    - ``v`` -- integer, the size of the point set for the design

    - ``k`` -- integer, the number of points per block

    - ``t`` -- integer, the size of sets covered by the blocks

    - ``verbose`` -- bool (default: ``False``), print verbose message

    OUTPUT:

    A :class:`CoveringDesign` object representing the ``(v, k, t)``-covering
    design with smallest number of blocks available in the database.

    EXAMPLES::

        sage: from sage.combinat.designs.covering_design import (
        ....:     best_known_covering_design_www)          # optional - internet
        sage: C = best_known_covering_design_www(7, 3, 2)  # optional - internet
        sage: print(C)                                     # optional - internet
        C(7,3,2) = 7
        Method: lex covering
        Submitted on: 1996-12-01 00:00:00
        0  1  2
        0  3  4
        0  5  6
        1  3  5
        1  4  6
        2  3  6
        2  4  5

    A ValueError is raised if the ``(v, k, t)`` parameters are not
    found in the database.
    """
    # import compatible with py2 and py3
    from six.moves.urllib.request import urlopen
    from sage.cpython.string import bytes_to_str
    from sage.misc.sage_eval import sage_eval

    v = int(v)
    k = int(k)
    t = int(t)

    param = "?v=%s&k=%s&t=%s" % (v, k, t)

    url = "http://ljcr.dmgordon.org/cover/get_cover.php" + param
    if verbose:
        print("Looking up the bounds at %s" % url)
    with urlopen(url) as f:
        s = bytes_to_str(f.read())

    if 'covering not in database' in s:  # not found
        str = "no (%d, %d, %d) covering design in database\n" % (v, k, t)
        raise ValueError(str)

    return sage_eval(s)

the function now works manually, i.e. I can do

sage: from sage.combinat.designs.covering_design import (
....:     best_known_covering_design_www)
sage: C = best_known_covering_design_www(7, 3, 2)
sage: print(C)
C(7, 3, 2) = 7
Method: lex covering
Submitted on: 1996-12-01 00:00:00
0  1  2
0  3  4
0  5  6
1  3  5
1  4  6
2  3  6
2  4  5

but, for some reason I cannot figure out, sage -t gives a NameError saying name 'best_known_covering_design_www' is not defined:

$ sage -t --optional=internet src/sage/combinat/designs/covering_design.py
Running doctests with ID 2019-03-08-21-29-22-a6581694.
Git branch: t-26592-covering-designs-www
Using --optional=internet
Doctesting 1 file.
sage -t --warn-long 81.7 src/sage/combinat/designs/covering_design.py
**********************************************************************
File "src/sage/combinat/designs/covering_design.py", line 514, in sage.combinat.designs.covering_design.best_known_covering_design_www
Failed example:
    C = best_known_covering_design_www(7, 3, 2)  # optional - internet
Exception raised:
    Traceback (most recent call last):
      File "/opt/s/sage/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/opt/s/sage/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 1095, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.designs.covering_design.best_known_covering_design_www[0]>", line 1, in <module>
        C = best_known_covering_design_www(Integer(7), Integer(3), Integer(2))  # optional - internet
    NameError: name 'best_known_covering_design_www' is not defined
**********************************************************************
File "src/sage/combinat/designs/covering_design.py", line 515, in sage.combinat.designs.covering_design.best_known_covering_design_www
Failed example:
    print(C)                                     # optional - internet
Exception raised:
    Traceback (most recent call last):
      File "/opt/s/sage/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/opt/s/sage/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 1095, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.designs.covering_design.best_known_covering_design_www[1]>", line 1, in <module>
        print(C)                                     # optional - internet
    NameError: name 'C' is not defined
**********************************************************************
1 item had failures:
   2 of   3 in sage.combinat.designs.covering_design.best_known_covering_design_www
    [2 tests, 2 failures, 0.01 s]
----------------------------------------------------------------------
sage -t --warn-long 81.7 src/sage/combinat/designs/covering_design.py  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 0.1 seconds
    cpu time: 0.0 seconds
    cumulative wall time: 0.0 seconds

Can somebody enlighten me?

comment:15 Changed 6 months ago by chapoton

  • do not break the import line
  • OR tag the first line of the import
  • OR use optional=sage,internet ?
Last edited 6 months ago by chapoton (previous) (diff)

comment:16 Changed 6 months ago by vklein

@slelievre with your patch and python 2 i obtain the following:

sage: best_known_covering_design_www(7, 3, 2)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-a930c6c24a1d> in <module>()
----> 1 best_known_covering_design_www(Integer(7), Integer(3), Integer(2))

<ipython-input-1-598f13fbcc94> in best_known_covering_design_www(v, k, t, verbose)
     55     if verbose:
     56         print("Looking up the bounds at %s" % url)
---> 57     with urlopen(url) as f:
     58         s = bytes_to_str(f.read())
     59 

AttributeError: addinfourl instance has no attribute '__exit__'

See: https://stackoverflow.com/questions/30627937/traceback-attributeerroraddinfourl-instance-has-no-attribute-exit/30628142

comment:17 Changed 6 months ago by vklein

  • Branch set to u/vklein/26592

comment:18 Changed 6 months ago by vklein

  • Commit set to 27eeb10c43f0a090de107ebd5f427ad26de3bf53

The py2 and py3 tests pass with branch u/vklein/26592.

I will now look for a way to avoid calling sage_eval on a internet given result.
It really looks like an highway for code injection to me.


New commits:

27eeb10Trac #26592: apply and fix slelievre's patch ...

comment:19 Changed 6 months ago by vklein

Constructing "manually" the CoveringDesign is not that easy because his __init__ function has eleven named parameters with default values, that mean there is a lot of potential string format that get_cover.php could return.

Do we know if get_cover.php always return a string in the form "CoveringDesign?(<all init parameters positionally>)" ?

comment:20 Changed 6 months ago by zerline

  • Cc zerline added

comment:21 Changed 6 months ago by vklein

  • Status changed from new to needs_info

comment:22 Changed 6 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:23 Changed 5 months ago by slelievre

  • Branch changed from u/vklein/26592 to u/slelievre/26592

comment:24 Changed 5 months ago by slelievre

  • Commit changed from 27eeb10c43f0a090de107ebd5f427ad26de3bf53 to e863721bac6eca92b83d17a220692c127dffc88d
  • Status changed from needs_info to needs_review

New branch based on Sage 8.8.beta2.

A first commit does pep8 reformatting + updating urls.

The second commit has the actual bytes_to_str fix in best_known_covering_design_www and a minor improvement to is_covering.

Please review.


New commits:

a1437e526592: Fix urls and do a round of pep8 in covering_designs.py
e86372126592: Fix py3 bytes_to_str + minor speedup

comment:25 Changed 5 months ago by embray

There is no point in these being in-line imports:

     from six.moves.urllib.request import urlopen
-
+    from sage.cpython.string import bytes_to_str

Those modules are always loaded by Sage so they might as well go at the module-level.

comment:26 Changed 5 months ago by git

  • Commit changed from e863721bac6eca92b83d17a220692c127dffc88d to a69f4ecdf33625f037bbad1375c6b3f24a5b5f12

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

a69f4ec26592: Move imports to beginning of file

comment:27 Changed 5 months ago by slelievre

Like this?

comment:28 follow-up: Changed 5 months ago by chapoton

This line:

https://drive.google.com/file/d/0B6s9_JNV1EjtUFd4MkhBc0hSdVU/view

is not something that we should link to.

Otherwise, looks good. In the future, please avoid doing large scale pep8 cleanup at the same time as serious changes.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 5 months ago by slelievre

Replying to chapoton:

This line:

https://drive.google.com/file/d/0B6s9_JNV1EjtUFd4MkhBc0hSdVU/view

is not something that we should link to.

Other options I can see are the following:

Otherwise, looks good. In the future, please avoid doing large scale pep8 cleanup at the same time as serious changes.

Sorry about that, I got slightly carried away. I tried to use a different commit for each type of changes, but probably could have done a better job of that, or used different tickets altogether. Thanks for reviewing anyway and for the advice for the future.

comment:30 Changed 5 months ago by slelievre

Or we could link to the copy archived at the Internet Archive's Wayback Machine:

comment:31 in reply to: ↑ 29 Changed 5 months ago by chapoton

Replying to slelievre:

Replying to chapoton:

This line:

https://drive.google.com/file/d/0B6s9_JNV1EjtUFd4MkhBc0hSdVU/view

is not something that we should link to.

Other options I can see are the following:

Yes, this one is better. One should also give the title (Coverings, chapter 1 of whatever)

comment:32 Changed 5 months ago by dgordon

Having my papers on google drive was the last part of my website that I hadn't updated. I've moved them now, so that paper can now be linked to at https://www.dmgordon.org/papers/hcd.pdf.

comment:33 Changed 5 months ago by git

  • Commit changed from a69f4ecdf33625f037bbad1375c6b3f24a5b5f12 to 1f3afa8ed60c5ab665a2d7ed3421323fe21dd6c1

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

1f3afa8Update Gordon--Stinson bibliographic reference

comment:34 Changed 5 months ago by slelievre

Updated now, please review.

comment:35 Changed 5 months ago by chapoton

  • Authors set to Samuel Lelièvre
  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, thx

comment:36 Changed 4 months ago by vbraun

  • Branch changed from u/slelievre/26592 to 1f3afa8ed60c5ab665a2d7ed3421323fe21dd6c1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:37 Changed 4 months ago by slabbe

  • Commit 1f3afa8ed60c5ab665a2d7ed3421323fe21dd6c1 deleted

With 8.8.beta5, I get:

sage: C = designs.best_known_covering_design_from_LJCR(7, 3, 2)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-1-a3d9f9e592f1> in <module>()
----> 1 C = designs.best_known_covering_design_from_LJCR(Integer(7), Integer(3), Integer(2))

/home/slabbe/GitBox/sage/local/lib/python2.7/site-packages/sage/combinat/designs/covering_design.pyc in best_known_covering_design_www(v, k, t, verbose)
    525     if verbose:
    526         print("Looking up the bounds at %s" % url)
--> 527     with urlopen(url) as f:
    528         s = bytes_to_str(f.read())
    529     if 'covering not in database' in s:  # not found

AttributeError: addinfourl instance has no attribute '__exit__'

Were you obtaining the same issue with this branch?

comment:38 Changed 4 months ago by vklein

I think so with urelopen(url) as... syntax has been introduced with commit e863721.

It should be avoided (see comment:16)

comment:39 Changed 4 months ago by vklein

Open a new ticket to fix that #27821

Note: See TracTickets for help on using tickets.