Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeError: cannot picke '_io.TextIOWrapper' regression in 0.30 for type=FileType(..), default=sys.stdin) #204

Closed
mfussenegger opened this issue Oct 23, 2023 · 2 comments
Assignees
Milestone

Comments

@mfussenegger
Copy link

mfussenegger commented Oct 23, 2023

It looks like there was a regression in the assembling refactor.
The following used to work:

import sys
import argh
from argparse import FileType


@argh.arg("-x", type=FileType("r", encoding="utf-8"), default=sys.stdin)
def foo(*, x):
    pass


if __name__ == "__main__":
    p = argh.ArghParser(prog="foo")
    p.add_commands([foo])
    p.dispatch()

Since 0.30.0 it results in the following stacktrace:

  File "/path/to/project/venv/lib/python3.11/site-packages/argh/helpers.py", line 47, in add_commands
    return add_commands(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/project/venv/lib/python3.11/site-packages/argh/assembling.py", line 557, in add_commands
    set_default_command(
  File "/path/to/project/venv/lib/python3.11/site-packages/argh/assembling.py", line 319, in set_default_command
    spec = _prepare_parser_add_argument_spec(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/path/to/project/venv/lib/python3.11/site-packages/argh/assembling.py", line 354, in _prepare_parser_add_argument_spec
    spec = ParserAddArgumentSpec(**asdict(parser_add_argument_spec))
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/dataclasses.py", line 1284, in asdict
    return _asdict_inner(obj, dict_factory)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/dataclasses.py", line 1291, in _asdict_inner
    value = _asdict_inner(getattr(obj, f.name), dict_factory)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/dataclasses.py", line 1325, in _asdict_inner
    return copy.deepcopy(obj)
           ^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/copy.py", line 161, in deepcopy
    rv = reductor(4)
         ^^^^^^^^^^^
TypeError: cannot pickle '_io.TextIOWrapper' object

I suspect it can't picke sys.stdin?


Side note: Was it intentional that the keyword-only arguments become already mandatory?
#191 mentioned a smooth transition path.

If you change the above example to:

@argh.arg("-x", type=FileType("r", encoding="utf-8"), default=sys.stdin)
def foo(x):
    pass

It fails with argh.exceptions.AssemblingError: foo: argument "x" declared as positional (in function signature) and optional (via decorator)

Not that big of a deal and easy enough to adjust, but given that the issue mentions that there should be a smooth migration I wanted to mention it too.

@neithere neithere added this to the 0.31 milestone Oct 23, 2023
@neithere neithere self-assigned this Oct 23, 2023
@neithere
Copy link
Owner

Hi @mfussenegger, thank you for the detailed report!

Confirming the regression. The fix is ready, to be released as 0.30.1 today or later this week.


Re not-so-smooth migration to the new name mapping policy: very good point, I've added a more informative error there with a hint to the possible solution.

FYI, I was indeed planning a smooth transition but the path with deprecation warnings would mean the necessity to update absolutely every app with an explicit policy and then deprecate the legacy policy. It would be far longer and require much more intervention from app developers. This way they have three options: a) stay at v0.29, b) upgrade but stay with the legacy policy, c) upgrade and add that asterisk.

Anyway, thanks for the valuable feedback, I was a bit nervous about this one when preparing the release 😄

@mfussenegger
Copy link
Author

Re not-so-smooth migration to the new name mapping policy: very good point, I've added a more informative error there with a hint to the possible solution.

I think part of the problem was that I first looked at the Github release (https://github.com/neithere/argh/releases/tag/v0.30.0). That led me to #199

If I had looked at the changes file it would've been much clearer. I thought maybe there was another issue preventing the smooth migration from working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants