gh-144782: Make sure that ArgumentParser instances are pickleable#144783
gh-144782: Make sure that ArgumentParser instances are pickleable#144783mauvilsa wants to merge 10 commits intopython:mainfrom
Conversation
|
All commit authors signed the Contributor License Agreement. |
Misc/NEWS.d/next/Library/2026-02-13-14-20-10.gh-issue-144782.0Y8TKj.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Bartosz Sławecki <bartosz@ilikepython.com>
|
Recent changes in 3.14.3 is making jsonargparse tests fail, see actions/runs/21987177775. Would be good to backport this to 3.14, otherwise pytorch-lightning multiprocessing will be broken on 3.14. From what I understood in the docs this pull request should have a |
Misc/NEWS.d/next/Library/2026-02-13-14-20-10.gh-issue-144782.0Y8TKj.rst
Outdated
Show resolved
Hide resolved
…Y8TKj.rst Co-authored-by: AN Long <aisk@users.noreply.github.com>
Bug fixes are backported, yes (to 3.13 as well at the time of writing). The labels can be applied at any point, even after merge. I'm leaving the decision whether we backport this or not to @savannahostrowski -- I'm not sure if this is a bug fix and not a feature, but it seems very reasonable to backport it, so you have a +1 from me in any case! |
Co-authored-by: Bartosz Sławecki <bartosz@ilikepython.com>
Co-authored-by: Bartosz Sławecki <bartosz@ilikepython.com>
There was a problem hiding this comment.
I filed mauvilsa#1 with some final suggestions that came to my mind again. Please consider merging them, and that will be all from me :-) The changes look good to me, thanks!
…stions Some documentation suggestions for pythonGH-144783
savannahostrowski
left a comment
savannahostrowski
left a comment
There was a problem hiding this comment.
I think this makes sense to fix, especially now that the workarounds don't work as a result of changes in 3.14. I think this is sufficiently low risk so I'm okay with backporting it.
However, can we frame this as bug/regression and not a new feature? To me, what we're doing here is just making it so that we aren't blocking pickling, not adding a new feature. As such, I don't think this warrants a versionchanged entry, and I'd opt to reframe the news entry accordingly.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
@mauvilsa, I've prepared mauvilsa#2 adding the requested changes. |
|
I think we should leave 3.13 as is, since that still works in jsonargparse. We just made changes to colorization in 3.14 that fundamentally break this and exacerbate the issue. |
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
I am not sure that we can guarantee that ArgumentParser is pickleable, with any combination of arguments. And it would be difficult to test this -- we would need to run each existing test with pickled/unpickled ArgumentParser (with all 6 protocols). This would make the tests an order of magnitude slower; This is too costly.
But I see nothing wrong in fixing a regression in 3.14 and making ArgumentParser more pickle-friendly by default. But this is not officially supported feature. So, I think it would be better to not add a versionchanged directive.
|
|
||
| class TestArgumentParserPickleable(unittest.TestCase): | ||
|
|
||
| @force_not_colorized |
There was a problem hiding this comment.
Does it work with color?
There was a problem hiding this comment.
With this fix, currently it does work with color. Should we also test with color?
I agree that it is difficult to guarantee pickleablility, but running all existing tests with pickle seems unnecessary. What would make a parser unpickleable is just assigning an unpickleable object. So only need one test for each path of the code where objects could be assigned. Instead of more tests could also be part of the review process, watching out for locally defined functions and lambdas.
There was a problem hiding this comment.
We can add tests for each concrete case after we found that it breaks pickle. But we cannot guarantee that future changes (maybe in other part of the stdlib) will not break it for not tested cases.
For example, currently it works with color. But the color machinery is outside of the scope of the argparse module. So, the test with color is necessary, without it we cannot catch regression.
There was a problem hiding this comment.
Yes I understand. I can add the test with color. I can also analyze the code a bit to see if there are more tests cases that seem important to prevent regressions. Apart from this, is there any way for me to get notified when an argparse pull request in cpython is ready for review? I can be the one looking out for changes that could make parsers unpickleable.
5 participants
Changes so that
argparse.ArgumentParserinstances are pickleable and a unit test so that this behavior is preserved.Fixes issue #144782