Skip to content

lib/python/pygrass: fix Python 3.14 pickling error in grid tests#7257

Draft
SyedAhad01 wants to merge 41 commits intoOSGeo:mainfrom
SyedAhad01:fix-python314-pickling
Draft

lib/python/pygrass: fix Python 3.14 pickling error in grid tests#7257
SyedAhad01 wants to merge 41 commits intoOSGeo:mainfrom
SyedAhad01:fix-python314-pickling

Conversation

@SyedAhad01
Copy link
Copy Markdown
Contributor

Fixes PicklingError failures when running tests with Python 3.14.

Python 3.14 cannot pickle local functions defined inside other functions
when using multiprocessing. This fix moves all run_grid_module inner
functions to module-level helper functions and uses functools.partial
to pass parameters.

Related to #6104

@github-actions github-actions bot added Python Related code is in Python libraries tests Related to Test Suite labels Apr 1, 2026
@SyedAhad01 SyedAhad01 changed the title pygrass: fix Python 3.14 pickling error in grid tests pygrass: Fix Python 3.14 pickling error in grid tests Apr 1, 2026
SyedAhad01 and others added 5 commits April 2, 2026 03:52
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 1, 2026

Read our contribution guide, you'll see that most of the back and forth of precious shared CI time can be avoided by using precommit (or prek) to have most simple formatting checks already made.

And I assume you're using an AI or agent or something like that, and I saw you were in VS Code in another PR, so you can reread and stage each line to make sure you're only committing what is relevant to the PR. Touching unrelated lines only makes reviewing harder: the last PR together, it started with many many changes, whilst it ended up being less than 10 lines, two parts. Way easier when concise like that.

@SyedAhad01
Copy link
Copy Markdown
Contributor Author

Thank you for the feedback and the guidance. I will read the contribution guide and set up pre-commit before pushing further changes. I will also make sure to carefully review each line and only commit what is directly relevant to the fix

@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 1, 2026

Don't worry, your general approach is fine. There's already some examples in the repo of the pattern needed, that I've fixed as a proof of concept a while ago. And it looked a bit like that.

@SyedAhad01 SyedAhad01 changed the title pygrass: Fix Python 3.14 pickling error in grid tests pygrass.modules: fix Python 3.14 pickling error in grid tests Apr 2, 2026
@SyedAhad01 SyedAhad01 changed the title pygrass.modules: fix Python 3.14 pickling error in grid tests lib/python/pygrass: fix Python 3.14 pickling error in grid tests Apr 2, 2026
SyedAhad01 and others added 3 commits April 5, 2026 11:57
Co-authored-by: Edouard Choinière <27212526+echoix@users.noreply.github.com>
Co-authored-by: Edouard Choinière <27212526+echoix@users.noreply.github.com>
@SyedAhad01
Copy link
Copy Markdown
Contributor Author

After removing xfail_mp_spawn, all tests now fail with RuntimeError: Subprocess failed with exit code 1 on both macOS and Windows. It seems the issue is inside GridModule itself where the internal worker functions are not picklable under spawn. Could you point me to the example in the repo that shows the correct pattern for fixing this? I want to make sure I apply the right fix

@SyedAhad01
Copy link
Copy Markdown
Contributor Author

I investigated the failures further and it looks like the issue is not in the test wrapper but inside GridModule itself, likely due to multiprocessing spawn requiring top-level worker functions.

I’ll explore the GridModule implementation to identify non-picklable functions and propose a fix.

Please let me know if there is an existing pattern in the repo I should follow.

@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 6, 2026

For the tests, it’s about right. For the other part, I’m not sure there’s a pattern yet, and that is the core of the work, to investigate what’s happening under the hood. Debugging subprocesses is maybe a bit harder, but you probably did a little bit.

It will be easier for you if you manage to do that locally. That’s not the kind of task that you can do by iterating in a PR.

@echoix echoix marked this pull request as draft April 6, 2026 13:32
@SyedAhad01
Copy link
Copy Markdown
Contributor Author

Thank you for the feedback. I understand now I should debug locally using spawn mode instead of iterating in the PR. I will reproduce the issue locally on Windows with multiprocessing.set_start_method('spawn', force=True), find the root cause, and then submit a clean fix. I will convert back to "Ready for review" once the local debugging is done.

@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 7, 2026

I just had an idea. In some python code recently-ish (last two years), we ended up needing to pass around env/session around in order to be able to properly initialize c-based tools. Is it possible that one of the functions here need to have that too? Like the failures would’ve been because the environment variables are not defined in the new subrosse

@SyedAhad01
Copy link
Copy Markdown
Contributor Author

Thank you for the suggestion! I implemented the fix by passing the environment variables explicitly through the work tuple to cmd_exe. However, the tests are still failing with the same RuntimeError: Subprocess failed with exit code 1 on both macOS and Windows. I am continuing to investigate the issue locally to find the root cause.

SyedAhad01 and others added 4 commits April 8, 2026 14:38
Pass env explicitly to copy_groups so subprocess workers inherit
the correct GRASS session environment in spawn mode (macOS/Windows).
Fixes RuntimeError: Subprocess failed with exit code 1 in grid tests.
Pass env explicitly to copy_groups so subprocess workers inherit
the correct GRASS session environment in spawn mode (macOS/Windows).
Fixes RuntimeError: Subprocess failed with exit code 1 in grid tests.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libraries Python Related code is in Python tests Related to Test Suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants