Opened 6 years ago

Closed 6 years ago

#14483 closed defect (fixed)

Helpful message if GAP fails to load a package

Reported by: ncohen Owned by: jdemeyer
Priority: major Milestone: sage-5.10
Component: combinatorics Keywords:
Cc: dimpase, vbraun, malb Merged in: sage-5.10.beta1
Authors: Nathann Cohen Reviewers: Dmitrii Pasechnik
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by dimpase)

Very short patch ! I met that problem while trying to run

sage: sage.combinat.designs.block_design.WittDesign(24)

and I had no idea how to install a gap package in the copy of gap that Sage contains...

Nathann

Attachments (1)

trac_14483.patch (5.0 KB) - added by ncohen 6 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 6 years ago by ncohen

  • Status changed from new to needs_review

comment:2 follow-up: Changed 6 years ago by jferreira

This patch works for me, applied to sage 5.8:

sage: sage.combinat.designs.block_design.WittDesign(24)

Incidence structure with 24 points and 759 block

However, after being prompted to run install_package("gap_packages"), gap_packages-4.5.7 is found. After you updated the spkg was I supposed to find gap_packages-4.6.3 ?

Last edited 6 years ago by jferreira (previous) (diff)

comment:3 follow-up: Changed 6 years ago by dimpase

  • Status changed from needs_review to needs_info

how does this ticket relate to #14039 ?

comment:4 Changed 6 years ago by dimpase

  • Description modified (diff)
  • Status changed from needs_info to needs_review

removed http://www.steinertriples.fr/gap-4.6.3.p0.spkg from the ticket description.

comment:5 follow-up: Changed 6 years ago by dimpase

  • Status changed from needs_review to positive_review

Positive review - the patch works on Sage 4.5.9.beta5 just fine. GAP updates should be dealt with elsewhere.

comment:6 Changed 6 years ago by dimpase

  • Component changed from packages: standard to combinatorics
  • Type changed from enhancement to defect

comment:7 Changed 6 years ago by jdemeyer

  • Reviewers set to Dmitrii Pasechnik
  • Summary changed from Update of GAP, and a an helpful message it fails at loading a package to Helpful message if GAP fails to load a package

comment:8 in reply to: ↑ 2 Changed 6 years ago by ncohen

However, after being prompted to run install_package("gap_packages"), gap_packages-4.5.7 is found. After you updated the spkg was I supposed to find gap_packages-4.6.3 ?

Nononono, gap and gap_packages are different spkg. Besides, when you ask Sage to install a package for you it will download the latest "accepted" package, so not the one from this ticket :-)

Nathann

comment:9 in reply to: ↑ 3 Changed 6 years ago by ncohen

how does this ticket relate to #14039 ?

Oops...

Nathann

comment:10 in reply to: ↑ 5 Changed 6 years ago by ncohen

Positive review - the patch works on Sage 4.5.9.beta5 just fine.

Thaaaaaaanks !

GAP updates should be dealt with elsewhere.

Yepyep. Sorry 'bout that :-)

Nathann

comment:11 Changed 6 years ago by vbraun

Doesn't really matter here, but in general error messages should be kept brief. If you want to show the user some explanation or hints how to get around a potential pitfall you should use warnings so that it is only printed once during the session.

comment:12 Changed 6 years ago by ncohen

Oh, I see. Thanks :-)

Nathann

comment:13 Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work
sage -t devel/sage/sage/groups/perm_gps/permgroup.py
**********************************************************************
File "devel/sage/sage/groups/perm_gps/permgroup.py", line 152, in sage.groups.perm_gps.permgroup.load_hap
Failed example:
    sage.groups.perm_gps.permgroup.load_hap()
Exception raised:
    Traceback (most recent call last):
      File "/mazur/release/merger/sage-5.10.beta1/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 466, in _run
        self.execute(example, compiled, test.globs)
      File "/mazur/release/merger/sage-5.10.beta1/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 825, in execute
        exec compiled in globs
      File "<doctest sage.groups.perm_gps.permgroup.load_hap[0]>", line 1, in <module>
        sage.groups.perm_gps.permgroup.load_hap()
      File "/mazur/release/merger/sage-5.10.beta1/local/lib/python2.7/site-packages/sage/groups/perm_gps/permgroup.py", line 157, in load_hap
        gap.load_package("hap")
      File "/mazur/release/merger/sage-5.10.beta1/local/lib/python2.7/site-packages/sage/interfaces/gap.py", line 514, in load_package
        "install_package(\"gap_packages\")"%pkg)
    RuntimeError: Error loading Gap package hap. You may want to install a collection of GAP packages by installing the 'gap_packages' Sage spkg. If so, then run instal
l_package("gap_packages")
**********************************************************************

comment:14 Changed 6 years ago by jdemeyer

I agree with Volker that a long warning is better than a long error message.

comment:15 Changed 6 years ago by ncohen

Okayyyyyyyyyyyyyyyy....

comment:16 Changed 6 years ago by ncohen

What do you think of this ? Honestly I preferred the previous version... A warning just before an exception does not stand out at all ... :-/

Nathann

comment:17 Changed 6 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:18 Changed 6 years ago by jferreira

  • Status changed from needs_review to needs_work
sage -t devel/sage/sage/groups/perm_gps/permgroup.py    
**********************************************************************
File "/Applications/sage/devel/sage/sage/groups/perm_gps/permgroup.py", line 152:
    sage: sage.groups.perm_gps.permgroup.load_hap()
Exception raised:
    Traceback (most recent call last):
      File "/Applications/sage/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/Applications/sage/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/Applications/sage/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_1[2]>", line 1, in <module>
        sage.groups.perm_gps.permgroup.load_hap()###line 152:
    sage: sage.groups.perm_gps.permgroup.load_hap()
      File "/Applications/sage/local/lib/python/site-packages/sage/groups/perm_gps/permgroup.py", line 157, in load_hap
        gap.load_package("hap")
      File "/Applications/sage/local/lib/python/site-packages/sage/interfaces/gap.py", line 511, in load_package
        raise RuntimeError('Error loading Gap package %s'%pkg)
    RuntimeError: Error loading Gap package hap
**********************************************************************

comment:19 Changed 6 years ago by ncohen

  • Status changed from needs_work to needs_review

Fixed ! What do you think of this warning/exception thing, by the way ?

Nathann

comment:20 Changed 6 years ago by vbraun

I'm pretty sure the doctest will fail if you happen to have gap_packages installed (which includes HAP). How about we use gap.load_package("foobar") or something like that instead?

comment:21 Changed 6 years ago by ncohen

Such a doctest already exists in interfaces/gap.py. Do you think that we should remove this doctest from permgroup.py ? I think it make sense.

What do you think of this warning/exception thing ?

Nathann

comment:22 Changed 6 years ago by vbraun

The doctest for the load_package() isn't necessary if it is tested elsewhere, but it is the natural place in the docstring to point out that you have to install HAP. I'll let you decide if you want to keep it or not.

As I said already, helpful explanations should be warnings not errors. The error message should be less than a whole line if possible. Advanced users shouldn't be drowned in paragraphs of text for simple errors. We could make warnings more prominent (say, using colors or bold text), but thats for another ticket.

comment:23 Changed 6 years ago by ncohen

Then I will probaly remove the doctest.

My problem with exceptions and warnings is not so general : in that case, a warning is printed just before the exception is raised, and no one sees the warning as a result. It would make more sense to just raise an exception. I will try to make the error message shorter.

Nathann

Changed 6 years ago by ncohen

comment:24 Changed 6 years ago by ncohen

What about this ?

Nathann

comment:25 Changed 6 years ago by vbraun

  • Status changed from needs_review to positive_review

Fine with me.

comment:26 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.10.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.