Opened 6 years ago

Last modified 4 weeks ago

#16607 needs_work enhancement

Enforce keyword-only parameters, with deprecation

Reported by: gagern Owned by:
Priority: major Milestone: sage-9.3
Component: misc Keywords:
Cc: niles, chapoton, gh-mwageringel Merged in:
Authors: Martin von Gagern Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: u/gagern/ticket/16607 (Commits) Commit: 9755cda19f2d85caa01222f852a06831e0369f4e
Dependencies: Stopgaps:

Description (last modified by mkoeppe)

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

Change History (28)

comment:1 Changed 6 years ago by niles

  • Cc niles added

comment:2 follow-up: Changed 6 years ago by 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.

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.

comment:3 in reply to: ↑ 2 Changed 6 years ago by gagern

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.

comment:4 Changed 6 years ago by gagern

  • Branch set to u/gagern/ticket/16607
  • Created changed from 07/03/14 12:53:10 to 07/03/14 12:53:10
  • Modified changed from 07/03/14 18:53:13 to 07/03/14 18:53:13

comment:5 Changed 6 years ago by gagern

  • Commit set to dde4ad657e8d9628c56b8907055956475153e79b

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

In comment:16:ticket:15515 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:

dde4ad6Implement @keyword_only decorator.

comment:6 Changed 6 years ago by git

  • Commit changed from dde4ad657e8d9628c56b8907055956475153e79b to 28aed693f5bb360d6262f7e6e22e980ddc3b925e

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

28aed69Implement @keyword_only decorator.

comment:7 Changed 6 years ago by git

  • Commit changed from 28aed693f5bb360d6262f7e6e22e980ddc3b925e to 60811212e129fa377f1fe34b678b559a771bfbb4

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

6081121Implement @keyword_only decorator.

comment:8 Changed 6 years ago by gagern

  • Authors set to Martin von Gagern
  • Status changed from new to needs_review

comment:9 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:10 follow-up: Changed 6 years ago by jdemeyer

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.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 6 years ago by gagern

Replying to jdemeyer:

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

In comment:16:ticket:17234 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!

comment:12 Changed 6 years ago by jdemeyer

Nils, do you have any more comments about this?

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

comment:13 Changed 6 years ago by jdemeyer

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.

comment:14 in reply to: ↑ 11 Changed 6 years ago by jdemeyer

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.

comment:15 Changed 6 years ago by git

  • Commit changed from 60811212e129fa377f1fe34b678b559a771bfbb4 to 9b9373b2696e6a30fd83dd1664cde5937eb2fb94

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

e697d4bImplement @keyword_only decorator.
c073211Use PEP links; add note about Cython support.
9b9373bUse @keyword_only for graphics_array.save() and .show().

comment:16 Changed 5 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

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

comment:17 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work
  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

comment:18 Changed 5 years ago by git

  • Commit changed from 9b9373b2696e6a30fd83dd1664cde5937eb2fb94 to 3e1497cae2ec902de35e5e011a5df79826359626

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

3e1497cAdd import for keyword_only decorator

comment:19 Changed 5 years ago by gagern

I forgot the import. Won't have time to test this today. but hope to do so tomorrow.

comment:20 Changed 5 years ago by git

  • Commit changed from 3e1497cae2ec902de35e5e011a5df79826359626 to d5876624bd579c544b68361f9db288d9111bef62

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

df5c834Add doctest for keyword_only real life use case
d587662Fix output of optional plot test cases

comment:21 Changed 5 years ago by git

  • Commit changed from d5876624bd579c544b68361f9db288d9111bef62 to 9755cda19f2d85caa01222f852a06831e0369f4e

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

9755cdaRemove duplicate word

comment:22 Changed 5 years ago by gagern

  • Status changed from needs_work to needs_review

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

comment:23 Changed 3 years ago by mderickx

Needs to be rebased on sage8.1beta3

comment:24 Changed 3 years ago by mderickx

  • Status changed from needs_review to needs_work

comment:25 Changed 2 months ago by mkoeppe

  • Cc chapoton added
  • Milestone changed from sage-6.4 to sage-9.2

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

comment:26 Changed 6 weeks ago by mkoeppe

  • Cc gh-mwageringel added

Pointers to recent tickets: #29243, #30194

comment:27 Changed 4 weeks ago by mkoeppe

  • Description modified (diff)
  • Milestone changed from sage-9.2 to sage-9.3
  • Summary changed from Enforce keyword-only parameters to Enforce keyword-only parameters, with deprecation

comment:28 Changed 4 weeks ago by mkoeppe

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