v.surf.icw: Python3 and modern GRASS compatibility, fix bug introduced in trac # 2574

[HamishB]

Hi,

this patch updates the module to work on systems without python 2, gets rid of error messages from g.message if the message is empty, and fixes a missing-first-point data corruption bug that was introduced by the patch in trac # 2574.

[petrasovaa]

The newline in messages is not a current practice. Most of these messages could be verbose.

[petrasovaa]

I suggest reviewing the style guide, to avoid formatting issues, especially pre-commit is really helpful.

[echoix]

For clang-format, just update from the main branch (grass8), there was an issue yesterday with the action.

[HamishB]

The newline in messages is not a current practice. Most of these messages could be verbose.

The module can take a very long time to run -- r.cost generates a map for every vector input point. So the status messages are in practice spread out in time and nice to see that something is actually happening. I've moved the individual vector point diagnostics into verbose messages, and the extra newlines were there to help make it more readable with those diagnostics so I've moved them to verbose mode too.

2c: I'm not a huge fan of pre-commit, I prefer to run those manually. I hadn't done it for the initial commit here as I was having trouble with black, but I've sorted that now. I assume requiring the very latest version of black is for a good reason, e.g. to avoid change-loops, but requiring an exact development environment is unfortunate.

[HamishB]

Also if I can ask a quick question- as far as backporting bugfixes to the grass7 -addons, should it be committed to the grass7 branch and the master branch ignored? Or send to both?

I seem to have a vague memory of some mention of the addons master branch being retired on one of the mailing lists a while ago but verrry vague memory.

[HamishB]

Nevermind. (#1098) :-)

[petrasovaa]

Not related to this PR, but since it runs a long time, could it be parallelized?

[petrasovaa]

See https://github.com/OSGeo/grass/blob/main/doc/development/style_guide.md#temporary-files

g.tempfile is meant for large data, in this case standard system tempfile seems better.

[petrasovaa]

See also how to deal with temporary maps in the guide. Right now you call cleanup function explicitly, but does it get called again since it's registered with atexit?

[HamishB]

See also how to deal with temporary maps in the guide. Right now you
call cleanup function explicitly, but does it get called again since it's
registered with atexit?

It gets called on exit a second time, but only tries to remove if something actually exists. It could be improved, tmp_base should be a global variable for one thing.

[HamishB]

See https://github.com/OSGeo/grass/blob/main/doc/development/style_guide.md#temporary-files

g.tempfile is meant for large data, in this case standard system tempfile seems better.

Any particular reason why? I don't really mind either way but g.tempfile only takes 1.36 milliseconds to complete for me, is only run twice during a long running module, doesn't pollute the filesystem outside of the mapset's .tmp dir, and gets a second chance to be cleared when the GRASS session ends/launches if the original core.try_remove() fails for some reason. g.tempfile just tries to create the file and reports back what its name is, it doesn't know or care about what ends up in it. For sure large files should use g.tempfile so that the disk space is used in MAPSET and we don't risk filling up a small /tmp partition.

[wenzeslaus]

Yes, g.tempfile is meant for large files and also for Bash/shells, but otherwise there is little reason to use it. 1) In general, relying on a standard library, in this case Python standard library, is better than relying on our custom mechanism. The library provides standardized, well-document and well-tested tools with known limits. 2) Security concerns are known and addressed. 3) Code analyzers can understand the code better. 4) Other readers need to know only standard Python, not GRASS GIS specific API. 5) Creating files in the mapset can be significantly slower than creating them in system's tmp dir. That dir is meant to be fast, that's at least what most application would expect. /tmp is sometimes mounted to memory. On the other hand, mapset dir may be on a slow network drive for storage of GRASS projects, large data, or because of parallel, multi-node processing in the same GRASS project.

Thinking about this more, some Python functions for tmp files and dirs allow you to pick the base tmp dir, so you can use the standard Python functions with mapset's .tmp if large files are a concern. (I used a custom dir approach to implement a temporary mapset session (TemporaryMapsetSession, create_temporary_mapset).) If that seems like a good way, we may want to document that g.tempfile is meant only for Bash/shells scripts and perhaps add a convenient wrapper for Python functions to work over mapset's tmp dir.

Another take on the situation is that the standard Python functions (and related code quality checks) are designed in a way that the code should be sure everything is properly deleted at the right time. The with statement is the prime example of that and that's what is usually appropriate and sufficient in individual scripts or tools (it is more difficult in the library where things should to be designed to work with the with statement, but may not be able use it themselves). This is different from the older approach of deleting on couple different levels or places in case the previous delete didn't work out or was forgotten.

[HamishB]

Not related to this PR, but since it runs a long time, could it be parallelized?

It is. :-)

# %option
# % key: workers
# % type: integer
# % options: 1-256
# % answer: 1
# % description: Number of parallel processes to launch
# %end

[neteler]

Please consider to use the G_OPT_M_NPROCS macro (https://github.com/OSGeo/grass/blob/997624b5bfa4e8141d5b742367d51fef97273efb/lib/gis/parser_standard_options.c#L757) which seems to fit.

[HamishB]

Please consider to use the G_OPT_M_NPROCS macro (https://github.com/OSGeo/grass/blob/997624b5bfa4e8141d5b742367d51fef97273efb/lib/gis/parser_standard_options.c#L757) which seems to fit.

Looks good, will do.

[wenzeslaus]

...requiring an exact development environment is unfortunate.

pre-commit is one way to overcome that. Virtual environment is another. The trend was so far that with each new version less changes are needed to existing code, so perhaps at some point, a reasonably recent version will be enough and the rest is caught by pre-commit or CI.

[wenzeslaus]

No need to limit the values. Definitively not the upper limit. 1- should work just fine.

There was some work done for C and OpenMP (not Python yet) to use zero and negative in a useful way.

[wenzeslaus]

Why the direct call to g.remove does not work without checking the existence first? Too verbose? There is superquiet=True if needed.

[echoix]

...requiring an exact development environment is unfortunate.

pre-commit is one way to overcome that. Virtual environment is another. The trend was so far that with each new version less changes are needed to existing code, so perhaps at some point, a reasonably recent version will be enough and the rest is caught by pre-commit or CI.

See the pyproject.toml here: where we only restrict black to require version 24 (they change the style once a year, and is stable otherwise)

https://github.com/OSGeo/grass/blob/7dc46cdadd53399c566cf0a496632c80ea03db81/pyproject.toml#L5-L6

Precommit manages its own virtual environments and updates according to the pre-commit config of the repo's branch transparently, so having the correct version pinned isn't a problem either.

So as long as you have a version of Black that does the 24 style, you're alright, otherwise there's a difference between the users of the repo.