Opened 6 years ago

Last modified 5 years ago

#19622 needs_work defect

Automatic error message when packages are not installed

Reported by: jdemeyer Owned by:
Priority: minor Milestone: sage-7.4
Component: user interface Keywords:
Cc: jmantysalo Merged in:
Authors: Jeroen Demeyer Reviewers:
Report Upstream: N/A Work issues: rebase
Branch: u/jdemeyer/better_error_message_when__bliss__is_not_installed (Commits, GitHub, GitLab) Commit: bd63cfa33ab37e8a67cac13298777008ef2d5acd
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

We add some magic to the Sage setup scripts such that OptionalExtensions whose package is not installed automatically raise an user-friendly exception:

sage: G = graphs.PetersenGraph()
sage: G.automorphism_group(algorithm='bliss') 
---------------------------------------------------------------------------
PackageNotFoundError                      Traceback (most recent call last)
<ipython-input-2-8e034558a836> in <module>()
----> 1 G.automorphism_group(algorithm='bliss')

/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/graphs/generic_graph.pyc in automorphism_group(self, partition, verbosity, edge_labels, order, return_group, orbits, algorithm)
  20062             if edge_labels:
  20063                 raise ValueError("bliss cannot be used when edge_labels is True")
> 20064             from sage.graphs.bliss import automorphism_group
  20065 
  20066             A = automorphism_group(self, partition)

PackageNotFoundError: the package 'bliss' was not found. You can install it by running 'sage -i bliss' in a shell

Change History (28)

comment:1 Changed 6 years ago by jdemeyer

  • Summary changed from Better error message when "bliss" is not installed to Better error message when packages are not installed

comment:2 Changed 6 years ago by jmantysalo

So what should we have on function level? A custom exception class inherited from ImportError, so that UI would detect it? Or raise ImportError with predefined format for exception string and little more complicated UI code to detect and parse it?

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

Jeroen created a PackageNotFoundError just for that. It should have been used there (most probably my fault).

comment:4 in reply to: ↑ 3 Changed 6 years ago by jmantysalo

Replying to ncohen:

Jeroen created a PackageNotFoundError just for that. It should have been used there (most probably my fault).

OK. Then when should ImportError be used?

comment:5 Changed 6 years ago by ncohen

I don't see a use case right now. It seems that when dealing with packages we should 'catch' the ImportError and raise PackageNotFoundError.

Nathann

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

comment:6 Changed 6 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Better error message when packages are not installed to Automatic error message when packages are not installed

comment:7 Changed 6 years ago by jdemeyer

  • Branch set to u/jdemeyer/better_error_message_when__bliss__is_not_installed

comment:8 Changed 6 years ago by jdemeyer

  • Commit set to 7d0ab29c3124acf96e066959ef80fe7cdcf0c8b9

This is a first draft, it still needs some doctests and maybe cleaning up.


New commits:

7d0ab29Automatic error message when importing an optional extension

comment:9 Changed 6 years ago by ncohen

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

comment:10 follow-up: Changed 6 years ago by ncohen

Is there a reason why you do it at such a low level? Wouldn't it be possible to just have the new 'fake' module just raise an exception in Python instead of doing it in C?

comment:11 follow-up: Changed 6 years ago by jmantysalo

But can this be customized? I.e. have something like "- - Please ask admin.email@… to install it." on maintained system.

comment:12 in reply to: ↑ 10 Changed 6 years ago by jdemeyer

Replying to ncohen:

Is there a reason why you do it at such a low level? Wouldn't it be possible to just have the new 'fake' module just raise an exception in Python instead of doing it in C?

I'm using C because it's actually the simplest solution. Otherwise I have to worry what happens is there is an .so and a .py file for the same module. Now I'm just replacing the sources of a C extension which exists anyway.

comment:13 in reply to: ↑ 11 ; follow-up: Changed 6 years ago by jdemeyer

Replying to jmantysalo:

But can this be customized?

Well, it's open source, so it can be customized :-)

What would you propose as user interface for customizing the message?

comment:14 in reply to: ↑ 13 Changed 6 years ago by jmantysalo

Replying to jdemeyer:

What would you propose as user interface for customizing the message?

SageNB already has Settings-tab when logged in as admin. For command line it could be a $SAGE_ROOT/messages.conf, by default a symlink to /etc/sage/messages.conf.

When this error happens the system should try to open $SAGE_ROOT for writing. If it can, then the message would be "...install with sage -i...". If not, it would open $SAGE_ROOT/messages.conf and print out the content. If that would not exists, then the default message would be something like "...ask your system administrator to...".

Rationale: Let's say we have users root, teacher and user. If user installs it, it would be used only for him/her. If root install, it will be on /usr/local/... and when installing new version the symlink would still point to same help text file. If teacher installs, and ask students to run ~teacher/sage/sage, he/she must manually make the configuration file after every reinstall.

comment:15 Changed 6 years ago by git

  • Commit changed from 7d0ab29c3124acf96e066959ef80fe7cdcf0c8b9 to 0c5c38a6808bb7cb4df7595d3370aa204e9bcd75

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

bdbb2c5Simplify code to get solver backend
c2d2809Simplify imports of optional packages
0c5c38aOnly write .c file if needed; add doctests

comment:16 Changed 6 years ago by jdemeyer

  • Status changed from new to needs_review

comment:17 follow-up: Changed 6 years ago by jdemeyer

Customizing the error message is outside the scope of this ticket, this just makes the raising of the exception automatic.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 6 years ago by jmantysalo

Replying to jdemeyer:

Customizing the error message is outside the scope of this ticket, this just makes the raising of the exception automatic.

OK. So this is off-topic, but still: Can you tell what to modify if I want to play with this kind of things? As an artificial example, print "Aarghs!" after division by zero.

comment:19 in reply to: ↑ 18 Changed 6 years ago by jdemeyer

Replying to jmantysalo:

OK. So this is off-topic, but still: Can you tell what to modify if I want to play with this kind of things?

See PackageNotFoundError in src/sage/misc/package.py.

As an artificial example, print "Aarghs!" after division by zero.

That's a different question since ZeroDivisionError is a builtin Python exception which we cannot just change. That's the advantage of using a custom exception class for PackageNotFoundError: you can easily change the string which is displayed.

Now, you can probably still play with IPython to customize displaying of exceptions in general, but then that's a very different story.

comment:20 Changed 6 years ago by ncohen

Thank you for the simplifications to the LP backend code.

comment:21 follow-up: Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Breaks patchbots?

comment:22 in reply to: ↑ 21 Changed 6 years ago by ncohen

Breaks patchbots?

That's a feature, not a bug.

comment:23 Changed 6 years ago by jdemeyer

I can see what the problem is. If you build this ticket, then you have some modules like sage.numerical.backends.coin_backend. Checking out a different branch doesn't make these modules go away.

comment:24 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:25 Changed 6 years ago by git

  • Commit changed from 0c5c38a6808bb7cb4df7595d3370aa204e9bcd75 to bd63cfa33ab37e8a67cac13298777008ef2d5acd

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

bd63cfaFix going back to older branches

comment:26 Changed 5 years ago by mkoeppe

  • Milestone changed from sage-6.10 to sage-7.4
  • Status changed from needs_review to needs_work
  • Work issues set to rebase

comment:27 follow-up: Changed 5 years ago by jdemeyer

Do you actually care about this ticket?

comment:28 in reply to: ↑ 27 Changed 5 years ago by mkoeppe

Replying to jdemeyer:

Do you actually care about this ticket?

Mildly. Less than I care about #6389, #21175, #21047, #19213, #18029, and below the threshold of rebasing it myself; but enough to look into it if there is a patch that applies.

Note: See TracTickets for help on using tickets.