Enforce keyword-only parameters, with deprecation

[gagern]

There are functions all over the library which accept various optional named parameters. Often the intention is that these will only be used as keyword parameters, but that fact is not enforced. Maintaining a huge number of possibly positional parameters can become a maintenance pain. (I'm currently seeing a mild version of this in #16533, but things could be much worse.)

PEP 3102 introduced syntax for this for Python 3.

This ticket introduces a decorator which will limit the number of positional parameters passed to its wrapped function.

1. To faciliate graceful deprecation, it might be associated with a trac ticket number and pass extra positional arguments after issuing a deprecation warning.

2. After the deprecation period, it would be replaced by proper keyword-only parameters.

Because of the performance penalty of the decorator, this should be done only for non-performance critical functions.

We demonstrate the use of the decorator on this ticket with a number of examples:

• get_solver - this would catch the typical user error get_solver('GLPK')

• more TBD.

CC: @nilesjohnson @fchapoton @mwageringel

Component: misc

Author: Martin von Gagern

Branch/Commit: u/gagern/ticket/16607 @ 9755cda

Reviewer: Volker Braun

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

[nbruin]

comment:2

-1 for performance reasons. Calls in python are expensive and wrappers installed by decorators add a call level. And sage tends to have a lot of calls already, so these costs quickly add up.

There would be a way to have both: have a flag set at startup that determines whether the decorator puts a check in place or just returns the original function. I'm afraid that's too complicated to be used in practice, though, but you could try.

[gagern]

comment:3

Replying to @nbruin:

-1 for performance reasons. Calls in python are expensive and wrappers installed by decorators add a call level. And sage tends to have a lot of calls already, so these costs quickly add up.

I am thinking about things which are unlikely to be used in a tight loop. Front ends to costly operations, mostly. In those case, the extra cost would be negligible compared to the typical cost of the function execution. I certainly wouldn't want to add this decorator to every function that might syntactically qualify, exactly due to these performance issues. I'd say the decorator should only be used if there is considerable gain associated with dropping a positional parameter.

There would be a way to have both: have a flag set at startup that determines whether the decorator puts a check in place or just returns the original function. I'm afraid that's too complicated to be used in practice, though, but you could try.

This might help when policing uses within Sage itself. My main concern however was user interaction. If we leave them an officially sanctioned option to disable the checks, then we might be responsible to maintain compatibility with deprecated syntax indefinitely. Unless we have the checks in place by default and make a very clear statement that people must assume responsibility if they decide to disable them. Checks enabled by default would change little in terms of performance for most users.

[gagern]

Branch: u/gagern/ticket/16607

[gagern]

Commit: dde4ad6

[gagern]

comment:5

OK, here is my implementation. With a big fat warning about performance considerations, to reduce the chances of inappropriate use.

In #15515 comment:16 I had to undo some changes by Jeroen Demeyer to avoid breaking compatibility for positional arguments. So it seems I'm not the only one who would like to get rid of some positional parameters to simplify implementations. With this tool here, we can have a proper deprecation period before we can finally dump them.

And with the extra keyword I introduced, things are even simpler for those cases. That keyword allows me to specify the names of any excess positional parameters, which will then be included in the **kwds dict so one doesn't have to mention them a second time when passing those arguments down the line.

---

New commits:

dde4ad6

Implement @keyword_only decorator.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

28aed69

Implement @keyword_only decorator.

[sagetrac-git]

Changed commit from dde4ad6 to 28aed69

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6081121

Implement @keyword_only decorator.

[sagetrac-git]

Changed commit from 28aed69 to 6081121

[gagern]

Author: Martin von Gagern

[jdemeyer]

comment:10

I feel like the issue in the ticket is not serious enough to require such a heavy-handed solution. So I'm leaning towards "wontfix".

Note that Cython files do actually support the ​PEP 3102 syntax even for Python 2.

[gagern]

comment:11

Replying to @jdemeyer:

I feel like the issue in the ticket is not serious enough to require such a heavy-handed solution.

In #17234 comment:16 you yourself had to remind people to not break positional parameter compatibility. So this looks like a recurring theme. And just because we have this tool here doesn't mean anyone will be forced to use it. But people may use it instead of inventing new workarounds at every turn.

Note that Cython files do actually support the ​PEP 3102 syntax even for Python 2.

That is good to know, thanks!

[jdemeyer]

comment:12

Nils, do you have any more comments about this?

For PEP links in Sphinx, you can use :pep:`3102` analogous to :trac:`16607`

[jdemeyer]

comment:13

Also: do you have a particular use-case in mind? If yes, please add this new decorator to an existing method, such that people can see how it is used in "real" situations.

[jdemeyer]

comment:14

Replying to @gagern:

Note that Cython files do actually support the ​PEP 3102 syntax even for Python 2.

That is good to know, thanks!

You should probably add this as a comment somewhere.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e697d4b	Implement @keyword_only decorator.
c073211	Use PEP links; add note about Cython support.
9b9373b	Use @keyword_only for graphics_array.save() and .show().

[sagetrac-git]

Changed commit from 6081121 to 9b9373b

[vbraun]

comment:16

Looks good to me. Its just a temporary measure anyways, we'll rip it out when we move to Python 3

[vbraun]

Reviewer: Volker Braun

[vbraun]

comment:17

  File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/plot/graphics.py", line 3403, in GraphicsArray
    @keyword_only(positional=4, deprecation=16607, extra="axes")
NameError: name 'keyword_only' is not defined
Makefile:60: recipe for target 'doc-html' failed
make: *** [doc-html] Error 1

[gagern]

comment:22

This looks better now. Sorry I hadn't tested this as well as I should have before.

[koffie]

comment:23

Needs to be rebased on sage8.1beta3

[mkoeppe]

comment:25

Now, after the switch to Python 3, we could revisit this.

[mkoeppe]

comment:26

Pointers to recent tickets: #29243, #30194

[mkoeppe]

comment:29

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:30

Setting a new milestone for this ticket based on a cursory review.