[bug#39734] scripts: Emit GC hint if free space is lower than absolute and relative threshold.

* guix/scripts.scm (%disk-space-warning-absolute): New variable.
(warn-about-disk-space): Test against %disk-space-warning-absolute.
Fix error in display-hint due to extraneous 'profile' argument.
---
 guix/scripts.scm | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

Hi!

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> * guix/scripts.scm (%disk-space-warning-absolute): New variable.
> (warn-about-disk-space): Test against %disk-space-warning-absolute.
> Fix error in display-hint due to extraneous 'profile' argument.

[...]

> +(define %disk-space-warning-absolute
> +  ;; The decimal number of GiB of free disk space below which a warning is
> +  ;; emitted.
> +  (make-parameter (match (and=> (getenv "GUIX_DISK_SPACE_WARNING_ABSOLUTE")
> +                                string->number)
> +                    (#f        17.0)
> +                    (threshold threshold))))

Perhaps we should arrange for ‘GUIX_DISK_SPACE_WARNING’ to handle both
cases?

That is, we’d first try to convert it with ‘size->number’; if that
works, it’s an absolute measure, and if it returns #f, then pass the
string to ‘string->number’ and assume it’s a fraction.

Does that make sense?

> +         (absolute-threshold-in-bytes (* 1024 1024 1024 absolute-threshold)))

Always use bytes internally; that is, convert to bytes at the UI border.

Thanks,
Ludo’.

Ludovic Courtès <ludo@gnu.org> writes:

> Hi!
>
> Pierre Neidhardt <mail@ambrevar.xyz> skribis:
>
>> * guix/scripts.scm (%disk-space-warning-absolute): New variable.
>> (warn-about-disk-space): Test against %disk-space-warning-absolute.
>> Fix error in display-hint due to extraneous 'profile' argument.
>
> [...]
>
>> +(define %disk-space-warning-absolute
>> +  ;; The decimal number of GiB of free disk space below which a warning is
>> +  ;; emitted.
>> +  (make-parameter (match (and=> (getenv "GUIX_DISK_SPACE_WARNING_ABSOLUTE")
>> +                                string->number)
>> +                    (#f        17.0)
>> +                    (threshold threshold))))
>
> Perhaps we should arrange for ‘GUIX_DISK_SPACE_WARNING’ to handle both
> cases?
>
> That is, we’d first try to convert it with ‘size->number’; if that
> works, it’s an absolute measure, and if it returns #f, then pass the
> string to ‘string->number’ and assume it’s a fraction.

I thought of something similar too, but this needs discussion:

- If we change the meaning of a value like "1", we are breaking backward compatibility.

- GUIX_DISK_SPACE_WARNING is currently undocumented, so I guess it's OK
  to break backward compatibility.  However, it'd be nice to document it :)

- Currently (size->number "1.MiB") "leaves" the Guile instance on error.  Which
  I find quite weird, I'd expect it to return #f when it cannot parse
  the input.

- Currently (size->number "0.8") returns 1.  If we want to use your
  suggestion, we would need to change the behaviour so that it returns
  #f.

- Alternatively, since we are breaking backward compatibility anyways,
  we could parse a trailing percent sign "%" to decide whether the value
  is relative or absolute.

Anyways, the issue was originally about dealing with both small and big
partitions, and for this we need both a default absolute threshold and a
default relative threshold.  Does that make sense?

>> +         (absolute-threshold-in-bytes (* 1024 1024 1024 absolute-threshold)))
>
> Always use bytes internally; that is, convert to bytes at the UI border.

What do you mean?

Hello!

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Hi!
>>
>> Pierre Neidhardt <mail@ambrevar.xyz> skribis:
>>
>>> * guix/scripts.scm (%disk-space-warning-absolute): New variable.
>>> (warn-about-disk-space): Test against %disk-space-warning-absolute.
>>> Fix error in display-hint due to extraneous 'profile' argument.
>>
>> [...]
>>
>>> +(define %disk-space-warning-absolute
>>> +  ;; The decimal number of GiB of free disk space below which a warning is
>>> +  ;; emitted.
>>> +  (make-parameter (match (and=> (getenv "GUIX_DISK_SPACE_WARNING_ABSOLUTE")
>>> +                                string->number)
>>> +                    (#f        17.0)
>>> +                    (threshold threshold))))
>>
>> Perhaps we should arrange for ‘GUIX_DISK_SPACE_WARNING’ to handle both
>> cases?
>>
>> That is, we’d first try to convert it with ‘size->number’; if that
>> works, it’s an absolute measure, and if it returns #f, then pass the
>> string to ‘string->number’ and assume it’s a fraction.
>
> I thought of something similar too, but this needs discussion:
>
> - If we change the meaning of a value like "1", we are breaking backward compatibility.

We can assume values <= 100 are a percentage.

> - GUIX_DISK_SPACE_WARNING is currently undocumented, so I guess it's OK
>   to break backward compatibility.  However, it'd be nice to document it :)

Yup.

> - Currently (size->number "1.MiB") "leaves" the Guile instance on error.  Which
>   I find quite weird, I'd expect it to return #f when it cannot parse
>   the input.

Fixed.

> - Currently (size->number "0.8") returns 1.  If we want to use your
>   suggestion, we would need to change the behaviour so that it returns
>   #f.

Hmm.

> - Alternatively, since we are breaking backward compatibility anyways,
>   we could parse a trailing percent sign "%" to decide whether the value
>   is relative or absolute.

Right.

> Anyways, the issue was originally about dealing with both small and big
> partitions, and for this we need both a default absolute threshold and a
> default relative threshold.  Does that make sense?

It does!

So how about this:

  We check for a trailing “%”, and if there’s one, assume it’s a percentage.

  Else, call ‘size->number’.  If we get an integer < 100, assume it’s a
  percentage, otherwise assume it’s an absolute size in bytes.

How does that sound?

Thanks!

Ludo’.

Ludovic Courtès <ludo@gnu.org> writes:

> So how about this:
>
>   We check for a trailing “%”, and if there’s one, assume it’s a percentage.
>
>   Else, call ‘size->number’.  If we get an integer < 100, assume it’s a
>   percentage, otherwise assume it’s an absolute size in bytes.
>
> How does that sound?

I think there is a misunderstanding.  If we have only _one_ threshold,
there is no way to set sane defaults for both big and small partitions.

Or were you thinking of something else?

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> So how about this:
>>
>>   We check for a trailing “%”, and if there’s one, assume it’s a percentage.
>>
>>   Else, call ‘size->number’.  If we get an integer < 100, assume it’s a
>>   percentage, otherwise assume it’s an absolute size in bytes.
>>
>> How does that sound?
>
> I think there is a misunderstanding.  If we have only _one_ threshold,
> there is no way to set sane defaults for both big and small partitions.

Oh, I must have misunderstood.

You mean you’d want to be able to set both an absolute threshold and a
relative threshold?  (Why “big and small” actually?)

Why not, but it seems to me we might be going overboard for something
that’s just meant as a hint to help people learn about ‘guix gc’.  :-)

Ludo’.

See the original discussion:

https://lists.gnu.org/archive/html/guix-devel/2020-02/msg00303.html

Our users fall into 2 categories:

- Those with a "small" store partition, say 25 GiB.
- Those with a "big" store partition, say 1 TiB.

If we use a relative threshold, users with a big partition will get
a hint all the time.  This may mislead newcomers into thinking that they
should run "guix gc" while they still have 50 GiB of free space.

If we use an absolute threshold, users with a small partition will get a
hint all the time.

In both cases, the hints becomes irrelevant if it's triggered for the
wrong reason.

The fix is to use both thresholds and to check against the lowest one.

Does that make more sense?

Cheers!

Hi!

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> See the original discussion:
>
> https://lists.gnu.org/archive/html/guix-devel/2020-02/msg00303.html

Oops, I’m lagging behind and had not seen it yet, sorry about that!

> Our users fall into 2 categories:
>
> - Those with a "small" store partition, say 25 GiB.
> - Those with a "big" store partition, say 1 TiB.
>
> If we use a relative threshold, users with a big partition will get
> a hint all the time.  This may mislead newcomers into thinking that they
> should run "guix gc" while they still have 50 GiB of free space.
>
> If we use an absolute threshold, users with a small partition will get a
> hint all the time.
>
> In both cases, the hints becomes irrelevant if it's triggered for the
> wrong reason.
>
> The fix is to use both thresholds and to check against the lowest one.
>
> Does that make more sense?

It does, yes.

So back to the original patch… I think we should use ‘size->number’ for
the absolute threshold.

I’m also still mildly reluctant to the addition of
‘GUIX_DISK_SPACE_WARNING_ABSOLUTE’ (one more variable and a weird
name—“absolute warning”?).  An option could be to allow
‘GUIX_DISK_SPACE_WARNING’ to optionally contain both thresholds.  These
would be valid values:

  1GiB;10%
  15            ;15% relative, and default absolute
  15%           ;same
  1G            ;1G absolute, and default relative

Thus ‘%disk-space-warning’ would hold a pair.

Does that make sense?

Apologies for the misunderstanding!

Ludo’.

Sounds good, I'll send a new patch as soon as possible! :)

Ludovic Courtès <ludo@gnu.org> writes:

> So back to the original patch… I think we should use ‘size->number’ for
> the absolute threshold.
>
> I’m also still mildly reluctant to the addition of
> ‘GUIX_DISK_SPACE_WARNING_ABSOLUTE’ (one more variable and a weird
> name—“absolute warning”?).  An option could be to allow
> ‘GUIX_DISK_SPACE_WARNING’ to optionally contain both thresholds.  These
> would be valid values:
>
>   1GiB;10%
>   15            ;15% relative, and default absolute
>   15%           ;same
>   1G            ;1G absolute, and default relative

The above logic implies that we test against size->number, and if it
fails then we set the relative threshold.

But a size->number quits Guile.  Should we error out instead?

Pierre Neidhardt <mail@ambrevar.xyz> writes:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> So back to the original patch… I think we should use ‘size->number’ for
>> the absolute threshold.
>>
>> I’m also still mildly reluctant to the addition of
>> ‘GUIX_DISK_SPACE_WARNING_ABSOLUTE’ (one more variable and a weird
>> name—“absolute warning”?).  An option could be to allow
>> ‘GUIX_DISK_SPACE_WARNING’ to optionally contain both thresholds.  These
>> would be valid values:
>>
>>   1GiB;10%
>>   15            ;15% relative, and default absolute
>>   15%           ;same
>>   1G            ;1G absolute, and default relative

Something else: in your example it's not logically possible to make out
if a raw number is relative or absolute.
So I suggest to break backward compatibility and do this:

   1GiB;10%
   15            ;15 absolute (15 bytes), and default relative
   15%           ;15% relative, and default absolute
   1G            ;1G absolute, and default relative

Thoughts?

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> So back to the original patch… I think we should use ‘size->number’ for
>> the absolute threshold.
>>
>> I’m also still mildly reluctant to the addition of
>> ‘GUIX_DISK_SPACE_WARNING_ABSOLUTE’ (one more variable and a weird
>> name—“absolute warning”?).  An option could be to allow
>> ‘GUIX_DISK_SPACE_WARNING’ to optionally contain both thresholds.  These
>> would be valid values:
>>
>>   1GiB;10%
>>   15            ;15% relative, and default absolute
>>   15%           ;same
>>   1G            ;1G absolute, and default relative
>
> The above logic implies that we test against size->number, and if it
> fails then we set the relative threshold.
>
> But a size->number quits Guile.

More precisely, it throws a 'quit exception.  :-)

So you could always do: (false-if-exception (size->number x)).

HTH,
Ludo’.

Update sent.

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> Pierre Neidhardt <mail@ambrevar.xyz> writes:
>
>> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>>>   1GiB;10%
>>>   15            ;15% relative, and default absolute
>>>   15%           ;same
>>>   1G            ;1G absolute, and default relative
>
> Something else: in your example it's not logically possible to make out
> if a raw number is relative or absolute.
> So I suggest to break backward compatibility and do this:
>
>    1GiB;10%
>    15            ;15 absolute (15 bytes), and default relative
>    15%           ;15% relative, and default absolute
>    1G            ;1G absolute, and default relative
>
> Thoughts?

What about having the rule that a number below 100 is a percentage, as
discussed before?  (It doesn’t make much sense to have an absolute
threshold of 42 bytes.)

Ludo’.

Ludovic Courtès <ludo@gnu.org> writes:

> What about having the rule that a number below 100 is a percentage, as
> discussed before?  (It doesn’t make much sense to have an absolute
> threshold of 42 bytes.)

Could do that.  Seems a bit ad-hoc though.

Patch updated.