Opened 7 years ago
Last modified 2 months ago
#16607 needs_work enhancement
Enforce keyword-only parameters, with deprecation
Reported by: | gagern | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.4 |
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, GitHub, GitLab) | Commit: | 9755cda19f2d85caa01222f852a06831e0369f4e |
Dependencies: | Stopgaps: |
Description (last modified by )
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.
- To faciliate graceful deprecation, it might be associated with a trac ticket number and pass extra positional arguments after issuing a deprecation warning.
- 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 (29)
comment:1 Changed 7 years ago by
- Cc niles added
comment:2 follow-up: ↓ 3 Changed 7 years ago by
comment:3 in reply to: ↑ 2 Changed 7 years ago by
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 7 years ago by
- 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 7 years ago by
- 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:
dde4ad6 | Implement @keyword_only decorator.
|
comment:6 Changed 7 years ago by
- Commit changed from dde4ad657e8d9628c56b8907055956475153e79b to 28aed693f5bb360d6262f7e6e22e980ddc3b925e
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
28aed69 | Implement @keyword_only decorator.
|
comment:7 Changed 7 years ago by
- Commit changed from 28aed693f5bb360d6262f7e6e22e980ddc3b925e to 60811212e129fa377f1fe34b678b559a771bfbb4
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
6081121 | Implement @keyword_only decorator.
|
comment:8 Changed 7 years ago by
- Status changed from new to needs_review
comment:9 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:10 follow-up: ↓ 11 Changed 7 years ago by
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: ↓ 14 Changed 6 years ago by
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
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
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
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
- Commit changed from 60811212e129fa377f1fe34b678b559a771bfbb4 to 9b9373b2696e6a30fd83dd1664cde5937eb2fb94
comment:16 Changed 6 years ago by
- 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 6 years ago by
- 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 6 years ago by
- Commit changed from 9b9373b2696e6a30fd83dd1664cde5937eb2fb94 to 3e1497cae2ec902de35e5e011a5df79826359626
Branch pushed to git repo; I updated commit sha1. New commits:
3e1497c | Add import for keyword_only decorator
|
comment:19 Changed 6 years ago by
I forgot the import. Won't have time to test this today. but hope to do so tomorrow.
comment:20 Changed 6 years ago by
- Commit changed from 3e1497cae2ec902de35e5e011a5df79826359626 to d5876624bd579c544b68361f9db288d9111bef62
comment:21 Changed 6 years ago by
- Commit changed from d5876624bd579c544b68361f9db288d9111bef62 to 9755cda19f2d85caa01222f852a06831e0369f4e
Branch pushed to git repo; I updated commit sha1. New commits:
9755cda | Remove duplicate word
|
comment:22 Changed 6 years ago by
- 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 4 years ago by
Needs to be rebased on sage8.1beta3
comment:24 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:25 Changed 9 months ago by
- 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 8 months ago by
- Cc gh-mwageringel added
comment:27 Changed 8 months ago by
- 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 8 months ago by
- Description modified (diff)
comment:29 Changed 2 months ago by
- Milestone changed from sage-9.3 to sage-9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
-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.