Skip to content
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

raster module memory: set new value globally #922

Merged
merged 7 commits into from
Aug 26, 2020
Merged

raster module memory: set new value globally #922

merged 7 commits into from
Aug 26, 2020

Conversation

neteler
Copy link
Member

@neteler neteler commented Aug 20, 2020

This change allows to globally override (as being written to $HOME/.grass7/rc) the default 300 MB raster cache setting with a user defined value:

r.in.gdal --help
Imports raster data into a GRASS raster map using GDAL library.
Usage:
 r.in.gdal [-ojeflakcrp] input=name output=name
[...]
Parameters:
           input   Name of raster file to be imported
          output   Name for output raster map
            band   Band(s) to select (default is all bands)
          memory   Maximum memory to be used (in MB)
                   default: 300                            <=== original
          target   Name of GCPs target location
           title   Title for resultant raster map
          offset   Offset to be added to band numbers
                   default: 0
[...]

# change to new setting, valid for all modules (r.cost, r.walk, i.segment, ...):
g.gisenv set="MEMORYMB=6000"

# example
r.in.gdal --help
Imports raster data into a GRASS raster map using GDAL library.
Usage:
 r.in.gdal [-ojeflakcrp] input=name output=name
[...]
Parameters:
           input   Name of raster file to be imported
          output   Name for output raster map
            band   Band(s) to select (default is all bands)
          memory   Maximum memory to be used (in MB)
                   default: 6000                            <=== !
          target   Name of GCPs target location
           title   Title for resultant raster map
          offset   Offset to be added to band numbers
                   default: 0
[...]

# set back to default
g.gisenv set="MEMORYMB=300"

Based on demo patch my @metzm - thanks!

Open issues:

a) Compiler warning (casting needed?):

main.c:195:12: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  195 |     memstr = G_getenv_nofatal("MEMORYMB");

b) How to do the same for the Python scripts?

$ ag 'answer: 300'
scripts/r.fillnulls/r.fillnulls.py
107:#% answer: 300

scripts/r.import/r.import.py
44:#% answer: 300

temporal/t.rast.import/t.rast.import.py
84:#% answer: 300


$ ag 'memory=300'
lib/python/temporal/stds_import.py
55:                                  set_current_region=False, memory=300):
176:                memory=300):

This change allows to globally override the default 300 MB with a user defined value:

r.in.gdal --help
Imports raster data into a GRASS raster map using GDAL library.

Usage:
 r.in.gdal [-ojeflakcrp] input=name output=name
...
Parameters:
           input   Name of raster file to be imported
          output   Name for output raster map
            band   Band(s) to select (default is all bands)
          memory   Maximum memory to be used (in MB)
                   default: 300
          target   Name of GCPs target location
           title   Title for resultant raster map
          offset   Offset to be added to band numbers
                   default: 0

g.gisenv set="MEMORYMB=6000"

r.in.gdal --help
Imports raster data into a GRASS raster map using GDAL library.

Usage:
 r.in.gdal [-ojeflakcrp] input=name output=name
...
Parameters:
           input   Name of raster file to be imported
          output   Name for output raster map
            band   Band(s) to select (default is all bands)
          memory   Maximum memory to be used (in MB)
                   default: 6000
          target   Name of GCPs target location
           title   Title for resultant raster map
          offset   Offset to be added to band numbers
                   default: 0

g.gisenv set="MEMORYMB=300"

Based upon demo patch my @metzm
@neteler neteler requested a review from metzm August 20, 2020 21:44
raster/r.cost/main.c Outdated Show resolved Hide resolved
raster/r.walk/main.c Outdated Show resolved Hide resolved
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a) Compiler warning (casting needed?):

main.c:195:12: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  195 |     memstr = G_getenv_nofatal("MEMORYMB");

C, GCC and/or Linux distros now take a lot of warnings more seriously than before, so we have a lot of these issues now.

In this case, defining memstr as const char* instead of char* should help. memstr can change, only the pointed to string cannot. (G_getenv_nofatal gives read-only pointers to an internal env data structure.)

b) How to do the same for the Python scripts?

$ ag 'answer: 300'
scripts/r.fillnulls/r.fillnulls.py
107:#% answer: 300

In theory, you could hack parser, that's a bad option, or you could make the interface definitions in Python dynamic as they are in C; that would be great, but I guess too much.

I think the best way to go about it is to define a standard option. This will move the default to the C level. It should work similarly to C only the module won't be able to set its own default which is possible in C with the current implementation.

Clearly, there is already a couple of those around, so standard option makes sense. It should also greatly reduce the duplication in your C code.

@neteler
Copy link
Member Author

neteler commented Aug 22, 2020

Thanks for your review @wenzeslaus !

a) Compiler warning (casting needed?):
...
In this case, defining memstr as const char* instead of char* should help. memstr can change, only the pointed to string cannot. (G_getenv_nofatal gives read-only pointers to an internal env data structure.)

ok I hope I got it right (the updated C code works and the compiler no longer complains (see lib/gis/parser_standard_options.c in this PR).

b) How to do the same for the Python scripts?

$ ag 'answer: 300'
scripts/r.fillnulls/r.fillnulls.py
107:#% answer: 300

In theory, you could hack parser, that's a bad option, or you could make the interface definitions in Python dynamic as they are in C; that would be great, but I guess too much.

I think the best way to go about it is to define a standard option. This will move the default to the C level. It should work similarly to C only the module won't be able to set its own default which is possible in C with the current implementation.

Clearly, there is already a couple of those around, so standard option makes sense. It should also greatly reduce the duplication in your C code.

Good suggestion, I have updated the PR accordingly and moved it all to G_define_standard_option(G_OPT_MEMORYMB).

While it should be all done for the C modules, it is yet not liked by Python:

e.g. (see the CI):

make[3]: Entering directory '/home/runner/work/grass/grass/scripts/r.import'
/usr/bin/install -c  r.import.py /home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/scripts/r.import
if [ "/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/scripts/r.import" != "" ] ; then GISRC=/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/demolocation/.grassrc79 GISBASE=/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu PATH="/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/bin:/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/bin:/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/scripts:$PATH" PYTHONPATH="/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/etc/python:/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/gui/wxpython:$PYTHONPATH" LD_LIBRARY_PATH="/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/bin:/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/bin:/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/scripts:/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/lib:/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/lib:/home/runner/install/lib" LC_ALL=C LANG=C LANGUAGE=C /home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/scripts/r.import --html-description < /dev/null | grep -v '</body>\|</html>' > r.import.tmp.html ; fi
Unknown option "output" in rule
WARNING: Bug in UI description. Missing option key
../../include/Make/Html.make:14: recipe for target 'r.import.tmp.html' failed
make[3]: *** [r.import.tmp.html] Error 1

Any pointers here?

@neteler
Copy link
Member Author

neteler commented Aug 22, 2020

Remaining error, I don't find where it comes from:

(from CI)

GISBASE="/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu" ARCH="x86_64-pc-linux-gnu" ARCH_DISTDIR="/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu" VERSION_NUMBER=7.9.dev VERSION_DATE=2020 python3 ./parser_standard_options.py -t "/home/runner/work/grass/grass/lib/gis/parser_standard_options.c" -f "grass" -o "/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/docs/html/parser_standard_options.html" -p 'id="opts_table" class="scroolTable"'
Traceback (most recent call last):
  File "./parser_standard_options.py", line 167, in <module>
    startswith=args.startswith))
  File "./parser_standard_options.py", line 89, in parse_options
    res = parse_glines(glines)
  File "./parser_standard_options.py", line 73, in parse_glines
    res[key].append(line[start:end])
AttributeError: 'str' object has no attribute 'append'
make[3]: *** [/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/docs/html/parser_standard_options.html] Error 1
make[3]: *** Waiting for unfinished jobs....
Makefile:127: recipe for target '/home/runner/work/grass/grass/dist.x86_64-pc-linux-gnu/docs/html/parser_standard_options.html' failed
make[3]: Leaving directory '/home/runner/work/grass/grass/man'
make[2]: *** [default] Error 2

@metzm
Copy link
Contributor

metzm commented Aug 23, 2020

The manuals are created at compile time, requiring a default answer to options. With your PR, the default answer can be changed after compile time, at runtime. This PR adjusts the default answer at runtime, but there is no default answer at compile time.
A patch to this PR, fixing the creation of man pages, could look like this:

index d8dba10d5..a0065a11a 100644
--- a/lib/gis/parser_standard_options.c
+++ b/lib/gis/parser_standard_options.c
@@ -138,7 +138,7 @@
 struct Option *G_define_standard_option(int opt)
 {
     struct Option *Opt;
-    const char *memstr;
+    char *memstr;
 
     Opt = G_define_option();
 
@@ -253,11 +253,13 @@ struct Option *G_define_standard_option(int opt)
 	Opt->key_desc = "memory in MB";
 	Opt->required = NO;
 	Opt->multiple = NO;
-	/* first check MEMORYMB in GISRC, set with g.gisenv */
-	memstr = G_getenv_nofatal("MEMORYMB");
-	if (!memstr)
-		memstr = "300";
-	Opt->answer = (char *) memstr;
+	Opt->answer = "300";
+	/* start dynamic answer */
+	/* check MEMORYMB in GISRC, set with g.gisenv */
+	memstr = G_store(G_getenv_nofatal("MEMORYMB"));
+	if (memstr && *memstr)
+	    Opt->answer = memstr;
+	/* end dynamic answer */
 	Opt->label = _("Maximum memory to be used (in MB)");
 	Opt->description = _("Cache size for raster rows");
 	break;
diff --git a/man/parser_standard_options.py b/man/parser_standard_options.py
index 5e2c753a5..6558bf421 100644
--- a/man/parser_standard_options.py
+++ b/man/parser_standard_options.py
@@ -50,7 +50,14 @@ def parse_options(lines, startswith='Opt'):
     def parse_glines(glines):
         res = {}
         key = None
+        dynamic_answer = False
         for line in glines:
+            if line.strip() == "/* start dynamic answer */":
+                dynamic_answer = True
+            if line.strip() == "/* end dynamic answer */":
+                dynamic_answer = False
+            if dynamic_answer or line.startswith('/*'):
+                continue
             if line.startswith('/*'):
                 continue
             if line.startswith(startswith) and line.endswith(';'):

This is a hard-coded hack to enable creation of manual pages.
Obviously, any changes to the GISRC variable MEMORYMB happening at runtime and can not be represented in the manuals.

- fixes the creation of man pages

#922 (comment)

Contributed by @metzm
@neteler
Copy link
Member Author

neteler commented Aug 24, 2020

The manuals are created at compile time, requiring a default answer to options. With your PR, the default answer can be changed after compile time, at runtime. This PR adjusts the default answer at runtime, but there is no default answer at compile time.

Ah, that's explaining it.

A patch to this PR, fixing the creation of man pages, could look like this:

Thanks so much, I have added your patch in 69367ff

This is a hard-coded hack to enable creation of manual pages.
Obviously, any changes to the GISRC variable MEMORYMB happening at runtime and can not be represented in the manuals.

... which is acceptable as MEMORYMB changing during compile time being rather unlikely, IMO.

@neteler neteler marked this pull request as ready for review August 24, 2020 17:03
@neteler neteler added the enhancement New feature or request label Aug 24, 2020
Copy link
Contributor

@metzm metzm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hack in parser_standard_options.py does not look very elegant, but then parser_standard_options.py is not a very elegant C code parser to start with. IMHO ready for merging.

@neteler
Copy link
Member Author

neteler commented Aug 26, 2020

The "Changes requested" isn't valid any more but I have no idea how to reset it...

@neteler neteler merged commit 0c4f02e into OSGeo:master Aug 26, 2020
@neteler neteler deleted the memory_grassrc_setting branch August 26, 2020 05:20
neteler added a commit that referenced this pull request Aug 31, 2020
* raster module memory: set other value globally

This change allows to globally override the default 300 MB with a user defined value:

r.in.gdal --help
Imports raster data into a GRASS raster map using GDAL library.

Usage:
 r.in.gdal [-ojeflakcrp] input=name output=name
...
Parameters:
           input   Name of raster file to be imported
          output   Name for output raster map
            band   Band(s) to select (default is all bands)
          memory   Maximum memory to be used (in MB)
                   default: 300
          target   Name of GCPs target location
           title   Title for resultant raster map
          offset   Offset to be added to band numbers
                   default: 0

g.gisenv set="MEMORYMB=6000"

r.in.gdal --help
Imports raster data into a GRASS raster map using GDAL library.

Usage:
 r.in.gdal [-ojeflakcrp] input=name output=name
...
Parameters:
           input   Name of raster file to be imported
          output   Name for output raster map
            band   Band(s) to select (default is all bands)
          memory   Maximum memory to be used (in MB)
                   default: 6000
          target   Name of GCPs target location
           title   Title for resultant raster map
          offset   Offset to be added to band numbers
                   default: 0

g.gisenv set="MEMORYMB=300"

Based upon demo patch my @metzm

* fix double description

* code simplification by defining new G_define_standard_option(G_OPT_MEMORYMB)

* update STD_OPT_STRINGS after include/gis.h modification

* Patch added to also set default answer at compile time

* document new GRASS variable MEMORYMB

- fixes the creation of man pages

#922 (comment)

Contributed by @metzm
wenzeslaus pushed a commit that referenced this pull request Jul 29, 2021
* This adds an NPROCS GIS environment variable.
* This change allows to globally override the default value (1) with a user defined value for the nprocs option using g.gisenv.
* This follows the existing implementation for memory with MEMORYMB variable (PR #922).
* nprocs option already exists, so this adds only the variable NPROCS and its documentation.
@neteler neteler added this to the 8.0.0 milestone Dec 9, 2021
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
* This adds an NPROCS GIS environment variable.
* This change allows to globally override the default value (1) with a user defined value for the nprocs option using g.gisenv.
* This follows the existing implementation for memory with MEMORYMB variable (PR OSGeo#922).
* nprocs option already exists, so this adds only the variable NPROCS and its documentation.
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
* This adds an NPROCS GIS environment variable.
* This change allows to globally override the default value (1) with a user defined value for the nprocs option using g.gisenv.
* This follows the existing implementation for memory with MEMORYMB variable (PR OSGeo#922).
* nprocs option already exists, so this adds only the variable NPROCS and its documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants