| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…to sentinels There were comments claiming these were implemented as custom classes to give a nicer repr(), but the repr() wasn't all that nice: >>> repr(dataclasses.MISSING) '<dataclasses._MISSING_TYPE object at 0x1005e7e00>' >>> repr(dataclasses.KW_ONLY) '<dataclasses._KW_ONLY_TYPE object at 0x100884050>' Sentinels are conceptually the right tool for these, so let's use them. This does change the repr() of these two objects.
|
Thanks, @JelleZijlstra. My only concern: is there any chance this will cause a performance regression when importing dataclasses? I assume not, since there are no imports involved, but I thought I'd ask. People are understandably touchy about it. |
Sorry, something went wrong.
|
It's almost certainly faster. Creating a class is quite slow; creating a sentinel is a very simple C call. From timeit it seems to be about 50x faster: >>> timeit.timeit("class X: pass")
2.916836875025183
>>> timeit.timeit("x = sentinel('x')")
0.06976754195056856
So this might actually measurably improve import time. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM! Thanks.
Sorry, something went wrong.
|
|
||
| def test_missing_repr(self): | ||
| self.assertIn('MISSING_TYPE object', repr(MISSING)) | ||
| self.assertEqual(repr(MISSING), 'MISSING') |
There was a problem hiding this comment.
I have no idea why the arguments were in the opposite order before, but changing them is a definite improvement!
Sorry, something went wrong.
There was a problem hiding this comment.
And yes, I realize that wasn't really the point of this change.
Sorry, something went wrong.
…3.15 See python/cpython#149086 and PEP 661.
…3.15 See python/cpython#149086 and PEP 661.
…3.15 See python/cpython#149086 and PEP 661.
…thon 3.15 (#4211) See python/cpython#149086 and [PEP 661](https://docs.python.org/3.15/whatsnew/3.15.html#whatsnew315-sentinel) for context. This fixes a traceback from `snakemake --help` in Python 3.15.0b1: ``` $ git clone https://github.com/snakemake/snakemake.git $ cd snakemake $ uv venv --python 3.15 $ . .venv/bin/activate (snakemake) $ uv pip install -e . (snakemake) $ snakemake --help Traceback (most recent call last): File "/usr/lib64/python3.15/argparse.py", line 2878, in print_help help_text = self.format_help(formatter=formatter) TypeError: ArgumentParser.format_help() got an unexpected keyword argument 'formatter' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/ben/src/forks/snakemake/src/snakemake/cli.py", line 2391, in main parser, args = parse_args(argv) ~~~~~~~~~~^^^^^^ File "/home/ben/src/forks/snakemake/src/snakemake/cli.py", line 1904, in parse_args args = parser.parse_args(argv) File "/home/ben/src/forks/snakemake/.venv/lib64/python3.15/site-packages/configargparse.py", line 1048, in parse_args args, argv = self.parse_known_args( ~~~~~~~~~~~~~~~~~~~~~^ args=args, ^^^^^^^^^^ ...<3 lines>... ignore_help_args=False, ^^^^^^^^^^^^^^^^^^^^^^^ ) ^ File "/home/ben/src/forks/snakemake/.venv/lib64/python3.15/site-packages/configargparse.py", line 1266, in parse_known_args namespace, unknown_args = argparse.ArgumentParser.parse_known_args( ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ self, args=args, namespace=namespace ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ) ^ File "/usr/lib64/python3.15/argparse.py", line 2126, in parse_known_args return self._parse_known_args2(args, namespace, intermixed=False) ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib64/python3.15/argparse.py", line 2155, in _parse_known_args2 namespace, args = self._parse_known_args(args, namespace, intermixed) ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib64/python3.15/argparse.py", line 2406, in _parse_known_args start_index = consume_optional(start_index) File "/usr/lib64/python3.15/argparse.py", line 2330, in consume_optional take_action(action, args, option_string) ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib64/python3.15/argparse.py", line 2231, in take_action action(self, namespace, argument_values, option_string) ~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib64/python3.15/argparse.py", line 1279, in __call__ parser.print_help() ~~~~~~~~~~~~~~~~~^^ File "/usr/lib64/python3.15/argparse.py", line 2882, in print_help help_text = self.format_help() File "/home/ben/src/forks/snakemake/.venv/lib64/python3.15/site-packages/configargparse.py", line 1696, in format_help return argparse.ArgumentParser.format_help(self) + ( ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^ File "/usr/lib64/python3.15/argparse.py", line 2839, in format_help return formatter.format_help() ~~~~~~~~~~~~~~~~~~~~~^^ File "/usr/lib64/python3.15/argparse.py", line 338, in format_help help = self._root_section.format_help() File "/usr/lib64/python3.15/argparse.py", line 263, in format_help item_help = join([func(*args) for func, args in self.items]) ~~~~^^^^^^^ File "/usr/lib64/python3.15/argparse.py", line 263, in format_help item_help = join([func(*args) for func, args in self.items]) ~~~~^^^^^^^ File "/usr/lib64/python3.15/argparse.py", line 612, in _format_action help_text = self._expand_help(action) File "/usr/lib64/python3.15/argparse.py", line 712, in _expand_help help_string = self._get_help_string(action) File "/home/ben/src/forks/snakemake/src/snakemake/common/argparse.py", line 76, in _get_help_string and not isinstance(action.default, dataclasses._MISSING_TYPE) ^^^^^^^^^^^^^^^^^^^^^^^^^ AttributeError: module 'dataclasses' has no attribute '_MISSING_TYPE' ``` The `TypeError` is supposed to happen, but it is supposed to be caught: https://github.com/python/cpython/blob/f31a89bb901067dd105b00cfa90523cf7ffdbbdd/Lib/argparse.py#L2877-L2882; that’s all in the Python standard library. But then the `AttributeError` on `_MISSING_TYPE` happens, and that’s the bug/regression in Snakemake. After this PR, `snakemake --help` works as expected. Trying again in a Python 3.14 virtualenv, I confirm that `snakemake --help` *still* works as expected. There are probably 100 different ways that this PR could have been written differently, and half of them are actually reasonable. I’m happy to consider style and implementation changes if requested. One thing I considered is that it might be just as good to always test against `dataclasses.MISSING`, which [already existed](https://docs.python.org/3.14/library/dataclasses.html#dataclasses.MISSING) with sentinel semantics even before the introduction of the PEP 661 `sentinel` type. I think that this probably works as expected, and it’s a lot simpler: ```diff diff --git a/src/snakemake/common/argparse.py b/src/snakemake/common/argparse.py index 1801419..db8d8b07 100644 --- a/src/snakemake/common/argparse.py +++ b/src/snakemake/common/argparse.py @@ -73,7 +73,7 @@ class ArgumentDefaultsHelpFormatter(argparse.HelpFormatter): or action.nargs in [argparse.OPTIONAL, argparse.ZERO_OR_MORE] ) and action.default not in (None, "", set(), argparse.SUPPRESS) - and not isinstance(action.default, dataclasses._MISSING_TYPE) + and not action.default is dataclasses.MISSING ): if isinstance(action.default, collections.abc.Iterable) and not isinstance( action.default, str ``` It’s worth considering that both of these approaches rely on implementation details of `dataclasses.MISSING`, and either of them may break at any point in the future. See the warning in https://docs.python.org/3.14/library/dataclasses.html#dataclasses.field that “No code should directly use the `MISSING` value.” Maybe taking a step back and reflecting on the bigger picture could reveal a better approach that sidesteps the issue completely. I’m not prepared to spend any more time on it right now, so please take this PR as a combined bug report and initial suggestion. <!--Add a description of your PR here--> ### QC <!-- Make sure that you can tick the boxes below. --> * [ ] The PR contains a test case for the changes or the changes are already covered by an existing test case. **To be honest, I don’t think there is anything in the test suite that checks that `snakemake --help` doesn’t crash, but I’m not prepared to contribute a test case. I welcome the addition of a relevant test case to this PR, but my own contribution will be limited to fixing the regression and confirming the fix manually.** * [x] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). **not necessary** * [x] I, as a human being, have checked each line of code in this pull request ### AI-assistance disclosure <!-- If AI tools were involved in creating this PR, please check all boxes that apply below and make sure that you adhere to our AI-assisted contributions policy: https://github.com/snakemake/snakemake/blob/main/docs/project_info/contributing.rst --> I used AI assistance for: * [ ] Code generation (e.g., when writing an implementation or fixing a bug) * [ ] Test/benchmark generation * [ ] Documentation (including examples) * [ ] Research and understanding *I did not use any AI assistance at any point in preparing this PR.* <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Ensure command-line help consistently recognizes and displays missing/default argument values across Python versions, preventing incorrect omission or duplication of default values in help text. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
| Back | FazBrowse Home | New Git URL |
There were comments claiming these were implemented as custom classes to give a nicer
repr(), but the repr() wasn't all that nice:
Sentinels are conceptually the right tool for these, so let's use them.
This does change the repr() of these two objects.