Feature.require vs. is_present, is_functional #33114

mkoeppe · 2022-01-04T18:43:09Z

(addressing slabbe's questions from #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.

CC: @seblabbe @orlitzky @kwankyu @saraedum

Component: refactoring

Author: Matthias Koeppe

Branch: f9232e8

Reviewer: Sébastien Labbé

Issue created by migration from https://trac.sagemath.org/ticket/33114

The text was updated successfully, but these errors were encountered:

saraedum · 2022-01-04T19:11:47Z

comment:1

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.

saraedum · 2022-01-04T19:13:38Z

comment:2

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.

seblabbe · 2022-01-04T19:44:07Z

comment:3

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 #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.

saraedum · 2022-01-04T20:58:14Z

comment:4

Replying to @seblabbe:

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 #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.)

mkoeppe · 2022-01-04T23:35:29Z

comment:5

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

seblabbe · 2022-01-05T09:48:13Z

comment:6

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 #33092 comment:27.

saraedum · 2022-01-05T16:55:40Z

comment:7

Replying to @seblabbe:

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 #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.

mkoeppe · 2022-01-05T18:52:29Z

comment:8

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

seblabbe · 2022-01-08T14:20:01Z

comment:9

Replying to @saraedum:

Replying to @seblabbe:

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 #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.

mkoeppe · 2022-01-09T18:40:41Z

comment:10

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

mkoeppe · 2022-01-09T18:44:59Z

comment:11

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.

mkoeppe · 2022-01-09T18:48:25Z

comment:12

Replying to @seblabbe:

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.

seblabbe · 2022-01-09T21:19:09Z

comment:13

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.

slel · 2022-01-12T09:00:30Z

comment:14

Maybe rename is_functional to _is_functional and is_presenttois_functional`?

seblabbe · 2022-01-13T08:06:13Z

comment:16

Replying to @slel:

Maybe rename is_functional to _is_functional and is_presenttois_functional`?

Personaly, I don't disagree.

mkoeppe · 2022-03-05T01:31:59Z

Dependencies: #31292

mkoeppe · 2022-03-06T00:58:12Z

Branch: u/mkoeppe/feature_require_vs__is_present__is_functional

mkoeppe · 2022-03-06T00:58:51Z

comment:19

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`

mkoeppe · 2022-03-06T00:58:51Z

Commit: 297e611

mkoeppe · 2022-03-06T00:58:51Z

Author: Matthias Koeppe

mkoeppe · 2022-03-06T00:59:12Z

comment:20

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

sagetrac-git · 2022-03-06T01:03:50Z

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

`21d4004`	`src/sage/features/imagemagick.py: Remove an is_functional doctest`
`f9232e8`	`src/sage/features/join_feature.py: Update doctest output with deprecation warning`

sagetrac-git · 2022-03-06T01:03:50Z

Changed commit from 297e611 to f9232e8

mkoeppe · 2022-03-06T01:04:34Z

comment:24

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

mkoeppe · 2022-03-17T18:16:51Z

Changed dependencies from #31292 to none

mkoeppe · 2022-03-25T16:37:36Z

comment:26

Let's get this in please

seblabbe · 2022-03-25T17:18:26Z

comment:27

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.

seblabbe · 2022-03-25T17:18:26Z

Reviewer: Sébastien Labbé

mkoeppe · 2022-03-25T17:38:04Z

comment:28

Thanks!

vbraun · 2022-03-30T22:33:12Z

Changed branch from u/mkoeppe/feature_require_vs__is_present__is_functional to f9232e8

seblabbe · 2022-03-31T09:25:20Z

Changed commit from f9232e8 to none

seblabbe · 2022-03-31T09:25:20Z

comment:30

updated description with choices made in this ticket

…ional deprecated in sagemath#33114 (2022)

…`sage.features`: Remove deprecated methods     - Fixes sagemath#35255 Removes methods deprecated in: - sagemath#31292 (2022) - sagemath#33114 (2022)  ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#37312 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee

mkoeppe added this to the sage-9.5 milestone Jan 4, 2022

mkoeppe added t: refactoring labels Jan 4, 2022

mkoeppe modified the milestones: sage-9.5, sage-9.6 Jan 12, 2022

mkoeppe added the s: needs review label Mar 6, 2022

mkoeppe added s: needs work and removed s: needs review labels Mar 6, 2022

mkoeppe added s: needs review and removed s: needs work labels Mar 6, 2022

seblabbe added s: positive review and removed s: needs review labels Mar 25, 2022

vbraun removed the s: positive review label Mar 30, 2022

vbraun closed this as completed in a54d7ce Mar 30, 2022

This comment has been minimized.

Sign in to view

vbraun mentioned this issue Mar 31, 2022

Modernize coercion for SpecialHyperellipticQuotientRing #33576

Closed

mkoeppe added a commit to mkoeppe/sage that referenced this issue Jun 8, 2024

src/sage/features/join_feature.py: Remove method JoinFeature.is_funct…

f7b1065

…ional deprecated in sagemath#33114 (2022)

Feature.require vs. is_present, is_functional #33114

Feature.require vs. is_present, is_functional #33114

Comments

mkoeppe commented Jan 4, 2022

saraedum commented Jan 4, 2022

saraedum commented Jan 4, 2022

seblabbe commented Jan 4, 2022

saraedum commented Jan 4, 2022

mkoeppe commented Jan 4, 2022

seblabbe commented Jan 5, 2022

saraedum commented Jan 5, 2022

mkoeppe commented Jan 5, 2022

seblabbe commented Jan 8, 2022

mkoeppe commented Jan 9, 2022

mkoeppe commented Jan 9, 2022

mkoeppe commented Jan 9, 2022

seblabbe commented Jan 9, 2022

slel commented Jan 12, 2022

seblabbe commented Jan 13, 2022

mkoeppe commented Mar 5, 2022

mkoeppe commented Mar 6, 2022

mkoeppe commented Mar 6, 2022

mkoeppe commented Mar 6, 2022

mkoeppe commented Mar 6, 2022

mkoeppe commented Mar 6, 2022

sagetrac-git mannequin commented Mar 6, 2022

sagetrac-git mannequin commented Mar 6, 2022

mkoeppe commented Mar 6, 2022

mkoeppe commented Mar 17, 2022

mkoeppe commented Mar 25, 2022

seblabbe commented Mar 25, 2022

seblabbe commented Mar 25, 2022

mkoeppe commented Mar 25, 2022

vbraun commented Mar 30, 2022

seblabbe commented Mar 31, 2022

seblabbe commented Mar 31, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.