#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: |
Description (last modified by )
(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 thatis_functional()
isTrue
? - should
is_present()
always returnFalse
whenis_functional
returnsFalse
(while the method names suggestis_present
could beTrue
whileis_functional
isFalse
)?
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 callsis_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
comment:2 Changed 6 months ago by
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: ↓ 4 Changed 6 months ago by
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.
comment:4 in reply to: ↑ 3 Changed 6 months ago by
Replying to slabbe:
Ok, this is how I understood the feature code up to today, but the meaning of
is_present
vsis_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
vsis_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: ↓ 6 Changed 6 months ago by
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: ↓ 7 Changed 6 months ago by
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: ↓ 8 ↓ 9 Changed 6 months ago by
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()
returnFalse
, 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 whetheris_present()
return True or False. Thus we need bothis_present_and_functional()
andis_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
Replying to saraedum:
I don't think you need both
is_functional
andis_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 aFeatureTestResult
to provide a lot of detail why something is not sufficient.
+1
comment:9 in reply to: ↑ 7 ; follow-up: ↓ 12 Changed 6 months ago by
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 theFeatureTestResult
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
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
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
Replying to slabbe:
we should not raise a
FeatureNotPresentError
and tell the user to install the feature, but rather raise aFeatureNotFunctionalError
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
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: ↓ 16 Changed 6 months ago by
Maybe rename is_functional
to `_is_functional
and is_present
to is_functional
?
comment:15 Changed 6 months ago by
- Milestone changed from sage-9.5 to sage-9.6
comment:16 in reply to: ↑ 14 Changed 6 months ago by
Replying to slelievre:
Maybe rename
is_functional
to `_is_functional andis_present
tois_functional
?
Personaly, I don't disagree.
comment:17 Changed 4 months ago by
- Dependencies set to #31292
comment:18 Changed 4 months ago by
- Branch set to u/mkoeppe/feature_require_vs__is_present__is_functional
comment:19 Changed 4 months ago by
- Commit set to 297e611d7a7457ff53d3d315e70cf0ad55005f45
- Status changed from new to needs_review
Here's a minimal change to improve the situation.
Last 10 new commits:
6c35717 | sage.features.FileFeature: Replace method absolute_path by absolute_filename, with deprecation
|
66d79f8 | sage.features.FileFeature.absolute_path: Fixup try...except...else
|
cad9f0f | sage.features.FileFeature.absolute_filename: New abstract method
|
57531d0 | src/sage/features/__init__.py: Document why importing sage.misc.superseded can fail
|
66b8125 | sage.features.FileFeature: Add doc
|
d631a03 | src/sage/features/__init__.py: Fix docstring markup
|
e9568e9 | FileFeature.absolute_filename: Fix docstring
|
ec97570 | Merge #31292
|
df2d065 | JoinFeature.is_functional: Deprecate
|
297e611 | Executable.is_functional: Document that it should not be called directly
|
comment:20 Changed 4 months ago by
(Last 2 commits only; it is on top of #31292)
comment:21 Changed 4 months ago by
- Status changed from needs_review to needs_work
comment:22 Changed 4 months ago by
- Commit changed from 297e611d7a7457ff53d3d315e70cf0ad55005f45 to f9232e8c15911f4a1434359d551372024db83f59
comment:23 Changed 4 months ago by
- Status changed from needs_work to needs_review
comment:24 Changed 4 months ago by
(Last 4 commits only; it's on top of #31292.)
comment:25 Changed 4 months ago by
- Dependencies #31292 deleted
comment:26 Changed 3 months ago by
Let's get this in please
comment:27 Changed 3 months ago by
- 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
Thanks!
comment:29 Changed 3 months ago by
- 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
- Commit f9232e8c15911f4a1434359d551372024db83f59 deleted
- Description modified (diff)
updated description with choices made in this ticket
comment:31 Changed 3 months ago by
- Description modified (diff)
comment:32 Changed 3 months ago by
- Description modified (diff)
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
callsis_present
which callsis_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 genericis_present
check. It should maybe be renamed to_is_functional
to make this more obvious.