[Bug] r.connectivity.corridors not installable on Windows

echoix commented 2 years ago

Name of the addon r.connectivity.corridors (from r.connectivity)

Describe the bug A clear and concise description of what the bug of the addon is.

To Reproduce Steps to reproduce the behavior:

First, fix the g.extension file to be able to download any extension, see comment https://github.com/OSGeo/grass/issues/2025#issuecomment-1003441506
Open GRASS' gui, and on the console tab, install extension with g.extension r.connectivity
See error

Expected behavior The install works, and the add-on is usable.

Screenshots

(Fri Dec 31 14:49:55 2021)                                                      
g.extension r.connectivity                                                      
Downloading precompiled GRASS Addons <r.connectivity>...
Fetching <r.connectivity> from <http://wingrass.fsv.cvut.cz/grass78/x86_64/addons/grass-7.8.6/r.connectivity.zip> (be patient)...
Updating extensions metadata file...
Updating extension modules metadata file...
WARNING: No metadata available for module 'r.connectivity.corridors': Unable to fetch interface description for command '<r.connectivity.corridors>'.

Details: <C:\OSGeo4W\bin\python3.exe: can't open file 'C:\Users\E-C\AppData\Local\Temp\grass7-E-C-19172\tmpgyj5byz5\r.connectivity.corridors': [Errno 2] No such file or directory
>
Installation of <r.connectivity> successfully finished
(Fri Dec 31 14:50:00 2021) Command finished (5 sec)

System description (please complete the following information):

Operating System: Windows 10

GRASS GIS version: 7.8.6

(Fri Dec 31 15:05:00 2021)                                                      
g.version -rge                                                                  
version=7.8.6
date=2021
revision=exported
build_date=2021-11-11
build_platform=x86_64-w64-mingw32
build_off_t_size=8
libgis_revision=2021-11-11T01:26:59+00:00
libgis_date=2021-11-11T01:26:59+00:00
proj=8.2.0
gdal=3.4.0
geos=3.10.0
sqlite=3.35.2
(Fri Dec 31 15:05:00 2021) Command finished (0 sec)

Additional context Before I found a way to work around the g.extension not being able to download any extensions, or use a different URL, I hard-coded a local path URL with the zip from https://wingrass.fsv.cvut.cz/grass78/x86_64/addons/grass-7.8.6/r.connectivity.zip downloaded, which gave the same error. In that zip file, we see that in the bin folder, there is missing a r.connectivity.corridors.bat file, so it is not a surprise that it is not possible to call r.connectivity.corridors with r.connectivity.corridors --interface-description to get the metadata, or calling that function directly. So I edited my local zip folder and added a bin/r.connectivity.corridors.bat with contents:

@"%GRASS_PYTHON%" "%GRASS_ADDON_BASE%/scripts/r.connectivity.corridors.py" %*

Then, it installed fine, but wasn't functional. When running, I get an error, but I figured out it is mainly due to an import error. Effectively, this packages has import resource which is Linux-only.

Further investigating this, I found that this is what might have caused the bat file from not appearing in the zip archive. When looking at the build logs of https://wingrass.fsv.cvut.cz/grass78/x86_64/addons/grass-7.8.6/logs/r.connectivity.log (date Mon, 11 Oct 2021 10:01:03 GMT), we see that there is an ModuleNotFoundError.

Traceback (most recent call last):
  File "C:\Users\landamar\grass_packager\grass786\addons\r.connectivity\scripts\r.connectivity.corridors.py", line 138, in <module>
    import resource
ModuleNotFoundError: No module named 'resource'

r.connectivity.log Same thing with https://wingrass.fsv.cvut.cz/grass74/x86_64/addons/grass-7.4.svn/logs/r.connectivity.log (date: Sun, 11 Aug 2019 11:47:36 GMT) and https://wingrass.fsv.cvut.cz/grass76/x86_64/addons/grass-7.6.1/logs/r.connectivity.log (date: Mon, 02 Sep 2019 21:36:47 GMT) for reference.

What is weird with this, is that the build status reports as SUCCESS, when it clearly fails, see https://wingrass.fsv.cvut.cz/grass78/x86_64/addons/grass-7.8.6/logs/#:~:text=raster/r.connectivity. But we see a list of errors, with the top of the file reporting no errors detected https://wingrass.fsv.cvut.cz/grass78/x86_64/logs/log-rd7d150202-1/error.log (date: Sat, 13 Nov 2021 14:43:46 GMT)

echoix commented 2 years ago

I investigated here how to workaround the resource module usage.

I see that the module is used to get ulimit, to activate z-flag if more maps have to be aggregated than ulimit. ulimit is set by

ulimit = resource.getrlimit(resource.RLIMIT_NOFILE)

So, I suggest the following solution:

First, protect the import from failing:

try:
    import resource
except ImportError:
    resource = None

Then, replace the ulimit assignment by using a default value, and calling the resource function if valid (limits inspired from https://stackoverflow.com/a/28212496):

    ulimit = (512, 2048) # Windows number of opened files (soft-limit, hard-limit)
    if resource:
        ulimit = resource.getrlimit(resource.RLIMIT_NOFILE)

I didn't create a pull request yet, since I can't get the r.connectivity.network to work (caused by rpy2), that creates the input for r.connectivity.corridors, so I can't fully test my changes.

Also, I'm skeptical that the protection wanted by using this condition does what is expected:

    # activate z-flag if more maps have to be aggregated than ulimit
    z_flag = None if len(selected_edges) < ulimit else "z"

By reading the answers of https://stackoverflow.com/q/6774724, we can see that the file limit is the total limit on files opened, including stdin, stdout, stderr, and other already opened files, so not just new files created by len(selected_edges) count in the limit.

ninsbl commented 2 years ago

Thanks for your feedback, @echoix !

Your suggestions regarding the resource module missing on MS Windows sound good to me. A PR would be great. However, I am currently working on r.connectivity (r.connectivity.network in particular, trying to remove the R dependency).

I do share your concerns that the check on the number of maps to be read against ulimit is not precise. There is probably no way to check that 100% exact and reliably (though I would be happy to be proven wrong here). So if you are operating with a number of maps around the ulimit, the check may fail. It should however catch obvious cases... The best way to tackle this is probably to add the z-flag to r.connectivity.corridors and give an error message (or warning with activating the z-flag) when the number of selected edges exceeds the uimit. Also, a corridor along an edge is defined by the distance betwen two patches, so one would rather need to know the number of patches selected than the number of edges...

echoix commented 2 years ago

Thanks! Great news for you working on the R dependency! I was going to try to tackle this when I finished reviewing the linguistic of add-ons, since I had some experience with R and much more with Python.

Perhaps you could help me on a particular question I have regarding the grass code structure. I tried looking into the code, but I still don't find where the dependencies (like python packages) are listed, and neither when or where they are installed. Coming from a different background (finished robotic engineering, with AI and deep learning), I only have about one semester of experience in geomatics, where I used the software more than developing it, I find it weird that it is not handled anywhere. And we see it causes problems, since tests can't run well, and I experienced it, where I was unable to use some GRASS add-ons. Do you know how it works in this project?

echoix commented 2 years ago

The best way to tackle this is probably to add the z-flag to r.connectivity.corridors and give an error message (or warning with activating the z-flag) when the number of selected edges exceeds the uimit. Also, a corridor along an edge is defined by the distance betwen two patches, so one would rather need to know the number of patches selected than the number of edges...

I don't fully understand how the z-flag works in the software yet, but could it be possible to add this flag during processing? For example, we could run normally, and when we hit the limit (have the error), then set the flag for the remaining layers? This way it could be more platform-independant. Or is batching a pool of lets say 100-200 file descriptors is performant enough?

neteler commented 2 years ago

Perhaps you could help me on a particular question I have regarding the grass code structure. I tried looking into the code, but I still don't find where the dependencies (like python packages) are listed, and neither when or where they are installed.

You can find the requirements of GRASS-code here:

Addons: Needed Python packages have to be handled individually. They can be installed with pip install python-PACKAGE. All addons should have a mechanism to notify the user if any needed package is missing.

wenzeslaus commented 2 years ago

...Addons: Needed Python packages have to be handled individually.

For sake of completeness, it should be noted that the desired state is that there is a standardized way of specifying dependencies, for example, requirement.txt file for Python packages which is than used during compilation, or that addons can be consumed by pip directly.

...They can be installed with pip install python-PACKAGE...

...or anything else if you document it like R in case of r.connectivity.corridors.

OSGeo / grass-addons

[Bug] r.connectivity.corridors not installable on Windows #657