Opened 6 months ago

Closed 3 months ago

Last modified 3 months ago

#33114 closed enhancement (fixed)

Feature.require vs. is_present, is_functional

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.6
Component: refactoring Keywords:
Cc: slabbe, mjo, klee, saraedum Merged in:
Authors: Matthias Koeppe Reviewers: Sébastien Labbé
Report Upstream: N/A Work issues:
Branch: f9232e8 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by slelievre)

(addressing slabbe's questions from ticket:33092#comment:23)

Some questions arise with respect to is_present(), is_functional() and require():

  • why does Feature.require() not check that is_functional() is True?
  • should is_present() always return False when is_functional returns False (while the method names suggest is_present could be True while is_functional is False)?

Choices made in this ticket:

  • only is_present() should be called from outside
  • it always checks that is_functional() returns True
  • the require() method only calls is_present()

A further step, not done in this ticket, would be, as suggested in comment:14, to rename is_functional to _is_functional, since only is_present is supposed to be used from outside.

Change History (32)

comment:1 Changed 6 months ago by saraedum

I believe that the original idea was to use this in the build process of SageMath. This influence some of the choices we made back then. If I understand correctly, that's not what we are planning to do anymore.

Anyway, require calls is_present which calls is_functional. At least that's the idea. As far as I recall, is_functional is just meant as a hook that you can override to add an additional check while relying on the generic is_present check. It should maybe be renamed to _is_functional to make this more obvious.

comment:2 Changed 6 months ago by saraedum

So, is_present means: is the feature present and functional.

Similarly, require means: require this feature to be present in the above sense.

I believe that the task here is to improve the documentation of these methods if that is not sufficiently clear.

comment:3 follow-up: Changed 6 months ago by slabbe

Ok, this is how I understood the feature code up to today, but the meaning of is_present vs is_functional always perturbed me.

When we speak with someone verbally, it is possible that we observe for example as in #33092 that imagemagick is "installed" on a machine, thus the convert command "is present", but it "is not functional" because it can't handle PNG files for instance. In real life, it happens that a feature is present but not functional. This is why the current design is weird a little (but probably easy to fix, for example by adding a new method is_present_and_functional() that would be used by require() and in the sagemath library.)

What I realised today in https://trac.sagemath.org/ticket/33092#comment:20 is that JoinFeature currently does not follow the same behavior with respect to is_present vs is_functional. Instead, it follows what we would be saying logically: a feature may be present but not functional.

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

comment:4 in reply to: ↑ 3 Changed 6 months ago by saraedum

Replying to slabbe:

Ok, this is how I understood the feature code up to today, but the meaning of is_present vs is_functional always perturbed me.

I'd propose to rename is_present to is_present_and_functional and is_functional to _is_functional. I agree that it's confusing.

What I realised today in https://trac.sagemath.org/ticket/33092#comment:20 is that JoinFeature currently does not follow the same behavior with respect to is_present vs is_functional. Instead, it follows what we would be saying logically: a feature may be present but not functional.

Yes, that is a bug in JoinFeature. (Copied over from the Latte feature from which it was created.)

comment:5 follow-up: Changed 6 months ago by mkoeppe

I agree with all of the above - except that it's perhaps not necessary to do the renaming is_present -> is_present_and_functional

comment:6 in reply to: ↑ 5 ; follow-up: Changed 6 months ago by slabbe

Replying to mkoeppe:

I agree with all of the above - except that it's perhaps not necessary to do the renaming is_present -> is_present_and_functional

I agree, I was thinking the same. When is_present_and_functional() return False, the first question that comes is whether it is because it is not functional or whether it is not present. Thus the next question to ask is whether is_present() return True or False. Thus we need both is_present_and_functional() and is_present().

Maybe the require() should raise a different type of error when it is not present vs not functional. Currently, when a feature is not functional the error message says the feature is not present which is weird. See https://trac.sagemath.org/ticket/33092#comment:27.

comment:7 in reply to: ↑ 6 ; follow-ups: Changed 6 months ago by saraedum

Replying to slabbe:

Replying to mkoeppe:

I agree with all of the above - except that it's perhaps not necessary to do the renaming is_present -> is_present_and_functional

I agree, I was thinking the same. When is_present_and_functional() return False, the first question that comes is whether it is because it is not functional or whether it is not present. Thus the next question to ask is whether is_present() return True or False. Thus we need both is_present_and_functional() and is_present().

So, is_functional was never meant to be exposed to the user. It should really be protected with a _. It is also only meant as a hook for the Executable feature. I would not change that. Similarly _is_present is not meant to be exposed to the user in any way.

I don't think you need both is_functional and is_present for the user. I don't think you really need to distinguish why a feature can't be used like that, i.e., in a machine-readable way. What would be your use case? Instead, the checks can return a FeatureTestResult to provide a lot of detail why something is not sufficient.

Maybe the require() should raise a different type of error when it is not present vs not functional. Currently, when a feature is not functional the error message says the feature is not present which is weird. See https://trac.sagemath.org/ticket/33092#comment:27.

Currently, require says not available. We could change that wording of course. The rest of the error message is pulled out of the FeatureTestResult if available.

comment:8 in reply to: ↑ 7 Changed 6 months ago by mkoeppe

Replying to saraedum:

I don't think you need both is_functional and is_present for the user. I don't think you really need to distinguish why a feature can't be used like that, i.e., in a machine-readable way. What would be your use case? Instead, the checks can return a FeatureTestResult to provide a lot of detail why something is not sufficient.

+1

comment:9 in reply to: ↑ 7 ; follow-up: Changed 6 months ago by slabbe

Replying to saraedum:

Replying to slabbe:

Maybe the require() should raise a different type of error when it is not present vs not functional. Currently, when a feature is not functional the error message says the feature is not present which is weird. See https://trac.sagemath.org/ticket/33092#comment:27.

Currently, require says not available. We could change that wording of course. The rest of the error message is pulled out of the FeatureTestResult if available.

Well, the full context of that line is FeatureNotPresentError: imagemagick is not available. When I read this and I try to interpret it, I understand that the command convert was simply not found on the machine. This is not the information that we want to give. The situation we have here is more tricky. The convert is available, the problem is that it does not work well. And here we should not say to the user "here is how to install convert" as we do currently.

Or, maybe, I am wrong on the meaning of the word feature or the word present? For you, what is the definition of a feature? For example, when ffmpeg is found in the PATH but can't produce a gif from a png. Is ffmpeg a feature? or rather ffmpeg + some garenty that it can do something? For me, in this case, the feature ffmpeg is present but it is not functional. Therefore we should not raise a FeatureNotPresentError and tell the user to install the feature, but rather raise a FeatureNotFunctionalError and tell some more meaningfull message to the user which needs to deal with this tricky situation.

comment:10 Changed 6 months ago by mkoeppe

Given that features map to optional tags, we should avoid semantics more complicated than "present" vs. "not present".

comment:11 Changed 6 months ago by mkoeppe

If there is an actual use case for distinguishing different levels of "presence" for a feature (present, functional, awesome, spiffy) - then it would be clearer to actually define separate features for these levels.

comment:12 in reply to: ↑ 9 Changed 6 months ago by mkoeppe

Replying to slabbe:

we should not raise a FeatureNotPresentError and tell the user to install the feature, but rather raise a FeatureNotFunctionalError and tell some more meaningful message to the user which needs to deal with this tricky situation.

+1 on better messages where possible, but I don't think it's useful to have a separate exception class.

comment:13 Changed 6 months ago by slabbe

Thanks for the answers. I agree to keep it simple and binary YES/NO. I do not have use cases for more levels of presence.

I allow myself to ask whether is_present() is still the good yes/no question we want to ask with the current design? Maybe a better name for it exists? Possibly is_present_and_functional() as suggested earlier? or something else?

I agree that my main point boils down to give good information to the user when require() fails.

comment:14 follow-up: Changed 6 months ago by slelievre

Maybe rename is_functional to `_is_functional and is_present to is_functional?

comment:15 Changed 6 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:16 in reply to: ↑ 14 Changed 6 months ago by slabbe

Replying to slelievre:

Maybe rename is_functional to `_is_functional and is_present to is_functional?

Personaly, I don't disagree.

comment:17 Changed 4 months ago by mkoeppe

  • Dependencies set to #31292

comment:18 Changed 4 months ago by mkoeppe

  • Branch set to u/mkoeppe/feature_require_vs__is_present__is_functional

comment:19 Changed 4 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to 297e611d7a7457ff53d3d315e70cf0ad55005f45
  • Status changed from new to needs_review

Here's a minimal change to improve the situation.


Last 10 new commits:

6c35717sage.features.FileFeature: Replace method absolute_path by absolute_filename, with deprecation
66d79f8sage.features.FileFeature.absolute_path: Fixup try...except...else
cad9f0fsage.features.FileFeature.absolute_filename: New abstract method
57531d0src/sage/features/__init__.py: Document why importing sage.misc.superseded can fail
66b8125sage.features.FileFeature: Add doc
d631a03src/sage/features/__init__.py: Fix docstring markup
e9568e9FileFeature.absolute_filename: Fix docstring
ec97570Merge #31292
df2d065JoinFeature.is_functional: Deprecate
297e611Executable.is_functional: Document that it should not be called directly

comment:20 Changed 4 months ago by mkoeppe

(Last 2 commits only; it is on top of #31292)

comment:21 Changed 4 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:22 Changed 4 months ago by git

  • Commit changed from 297e611d7a7457ff53d3d315e70cf0ad55005f45 to f9232e8c15911f4a1434359d551372024db83f59

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

21d4004src/sage/features/imagemagick.py: Remove an is_functional doctest
f9232e8src/sage/features/join_feature.py: Update doctest output with deprecation warning

comment:23 Changed 4 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:24 Changed 4 months ago by mkoeppe

(Last 4 commits only; it's on top of #31292.)

comment:25 Changed 4 months ago by mkoeppe

  • Dependencies #31292 deleted

comment:26 Changed 3 months ago by mkoeppe

Let's get this in please

comment:27 Changed 3 months ago by slabbe

  • Reviewers set to Sébastien Labbé
  • Status changed from needs_review to positive_review

works for me. Thanks for this improvement.

I think a next step further is, as suggested in comment:14, to rename is_functional to _is_functional since only is_present is supposed to be used from outside.

comment:28 Changed 3 months ago by mkoeppe

Thanks!

comment:29 Changed 3 months ago by vbraun

  • Branch changed from u/mkoeppe/feature_require_vs__is_present__is_functional to f9232e8c15911f4a1434359d551372024db83f59
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:30 Changed 3 months ago by slabbe

  • Commit f9232e8c15911f4a1434359d551372024db83f59 deleted
  • Description modified (diff)

updated description with choices made in this ticket

comment:31 Changed 3 months ago by slabbe

  • Description modified (diff)

comment:32 Changed 3 months ago by slelievre

  • Description modified (diff)
Note: See TracTickets for help on using tickets.