Skip to content

init: Add retries and timeout to mapset locking #5443

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

Merged
merged 18 commits into from
May 1, 2025

Conversation

wenzeslaus
Copy link
Member

@wenzeslaus wenzeslaus commented Mar 25, 2025

This change introduces a timeout parameter in CLI which is enabled by default and makes GRASS wait if the mapset is locked.

Additionally, this adds timeout parameter also to the Python API, so this keeps feature parity between the two (with locking and force lock removal already in place since #5591). The automatic tests are using the Python API, but they are limited as they don't use multiple threads or processes.

Locked mapset example in CLI:

$ grass ~/grassdata/nc_spm_08_grass7/user1/
Starting GRASS GIS...
Mapset <.../nc_spm_08_grass7/user1> locked (attempt 1), but will retry in 1.00 seconds...
Mapset <.../nc_spm_08_grass7/user1> locked (attempt 2), but will retry in 2.00 seconds...
Mapset <.../nc_spm_08_grass7/user1> locked (attempt 3), but will retry in 4.00 seconds...

Lock in Python (also works as a context manager):

import grass.script as gs
s = gs.setup.init("~/grassdata/nc_spm_08_grass7/user1/", lock=True, timeout=0)
s.finish()

This also introduces new subcommands to the experimental execution of the grass.app with CLI API using python -m grass.app from #5590. Specifically, lock and unlock subcommands are now available and can be used for testing (compensating for the limited automatic tests), fixing lock situations, and by external applications which need to persistently lock mapset without using the Python API directly. Subcommands are added to the first level, but could eventually be under a project or mapset subcommand.

Example lock-unlock (path to Python packages needs to be setup, don't ask me why LD_LIBRARY_PATH does not need to be setup - but it is needed for test of the Python API):

export PYTHONPATH="$(.../bin.x86_64-pc-linux-gnu/grass --config python-path)"
python -m grass.app lock ~/grassdata/nc_spm_08_grass7/user1/
python -m grass.app unlock ~/grassdata/nc_spm_08_grass7/user1/

Notably, the locking is disabled on Windows, so test is disabled for Windows.

@wenzeslaus wenzeslaus added the enhancement New feature or request label Mar 25, 2025
@wenzeslaus wenzeslaus added this to the 8.5.0 milestone Mar 25, 2025
@github-actions github-actions bot added Python Related code is in Python libraries docs markdown Related to markdown, markdown files tests Related to Test Suite labels Mar 25, 2025
@wenzeslaus
Copy link
Member Author

I need to fix the tests...there is no locking on Windows. Otherwise the tests pass.

@echoix
Copy link
Member

echoix commented Apr 23, 2025

Isn't this starting to be three PRs at once? At least for the cli changes, that are great by themselves. (I already explored similar changes like two years ago when starting to work on modernizing gunittest, syncing with upstream unittest, but wasn't completed as no way to fully validate if it there would be impacts on tests on all our platforms, as that the tests were not all running or would allow failures on some platforms). But the __main__.py approach is correct!

@echoix
Copy link
Member

echoix commented Apr 23, 2025

And if you are up to fixing some tests, maybe update the branch that is 108 commits behind, I’ve made some progress on this part recently. And more to come.

@wenzeslaus
Copy link
Member Author

wenzeslaus commented Apr 24, 2025

I can split that, adding the locking to init without a timeout. The reason I did only one is that init and CLI should have feature parity and the __main__.py CLI was just a good way of testing the changes.

@echoix
Copy link
Member

echoix commented Apr 24, 2025

From digging the history quite often, I know that following renames+changes are quite hard. The CLI main part could go right through, and then add on the options?

@wenzeslaus
Copy link
Member Author

From digging the history quite often, I know that following renames+changes are quite hard. The CLI main part could go right through, and then add on the options?

This PR does not have a rename, no?

@echoix
Copy link
Member

echoix commented Apr 24, 2025

Indeed, I looked at the commits of yesterday, and thought that 0a2887f was moving code around from __main__.py to cli.py. While for the PR, both are new files.

@wenzeslaus
Copy link
Member Author

I have now WIP to introduce the internal CLI with just simple help functionality and I'll later split also the Python locking addition to a separate PR.

wenzeslaus added a commit to wenzeslaus/grass that referenced this pull request Apr 25, 2025
This adds a CLI to grass.app subpackage using `__main__.py` and Python argparse with subcommands. At this point, this is meant as internal and experimental. The original motivation for this is testing locking in OSGeo#5443. The subcommands add a lot of flexibility in what can done with argparse supporting directly several use cases and being quite stable and standardized.

Just as an example (to have this separate from OSGeo#5443 while functional), this adds two subcommands, help and man, which both end up calling g.manual.
wenzeslaus added a commit that referenced this pull request Apr 29, 2025
This adds a CLI to grass.app subpackage using `__main__.py` and Python argparse with subcommands. At this point, this is meant as internal and experimental. The original motivation for this is testing locking in #5443. The subcommands add a lot of flexibility in what can done with argparse supporting directly several use cases and being quite stable and standardized.

Just as an example (to have this separate from #5443 while functional), this adds two subcommands, help and man, which both end up calling g.manual. These specific subcommands are not a replacement for calling g.manual inside an interactive shell, but there are a replacement for command line call with a temporary project setup.
@wenzeslaus
Copy link
Member Author

This is now ready for review again. It includes changes from #5590 (subcommands) and #5591 (locking in Python API).

@wenzeslaus wenzeslaus changed the title init: Add timeout when locking mapset, add locking to Python API init: Add retries and timeout to mapset locking May 1, 2025
@wenzeslaus wenzeslaus merged commit 79c4769 into OSGeo:main May 1, 2025
29 of 31 checks passed
@wenzeslaus wenzeslaus deleted the lock-with-wait-and-timeout branch May 1, 2025 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs enhancement New feature or request libraries markdown Related to markdown, markdown files Python Related code is in Python tests Related to Test Suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants