[Bug] grass.pygrass Module parameter checking is too strict

[wenzeslaus]

Describe the bug

The Module class from grass.pygrass does not understand parser rules for renaming and shortening which creates potential issues for backward compatibility and confusion when switching between command line and Python.

Details

The parser for command line interface of tools (modules) allows renamed options, renamed flags, shorter option names, and shorter option values, for example:

grass --tmp-mapset ~/grassdata/nc_spm_08_grass7/ --exec g.region raster='elevation'
grass --tmp-mapset ~/grassdata/nc_spm_08_grass7/ --exec g.region rast='elevation'
grass --tmp-mapset ~/grassdata/nc_spm_08_grass7/ --exec v.db.select map='hospitals' null_value='N/A'
grass --tmp-mapset ~/grassdata/nc_spm_08_grass7/ --exec v.db.select map='hospitals' nv='N/A'
grass --tmp-mapset ~/grassdata/nc_spm_08_grass7/ --exec v.to.rast input=lakes output=lakes use=value
grass --tmp-mapset ~/grassdata/nc_spm_08_grass7/ --exec v.to.rast input=lakes output=lakes use=val

The original motivation for this feature was keeping backward compatibility between v6 and v7 and keeping the convenience of short names when using command line interactively. The convenience still holds and the backward compatibility feature is potentially even more important when we want to fix things without waiting for a next major release.

The run_command family of functions from grass.script understands that (or simply leaves that to the parser implementation), for example:

grass --tmp-mapset ~/grassdata/nc_spm_08_grass7/ --exec python -c "import grass.script as gs; gs.run_command('g.region', rast='elevation')"

However, grass.pygrass.modules.Module does its own checking and it does not know about any of the features of parser, for example:

grass ... --exec python -c "... Module('g.region', rast='elevation')"
grass ... --exec python -c "... Module('v.db.select', map='hospitals', nv='N/A')"
grass ... --exec python -c "... Module('v.to.rast', input='lakes', output='lakes', use='value')"

The shortening for convenience is much less important in Python than in an interactive command line interface. Possibly, it is even promoting a better practice by requiring full names. However, the backward compatibility is not supported rendering every change backward incompatible regardless of the parser system in place which is addressing the issue.

To Reproduce

grass --tmp-mapset ~/grassdata/nc_spm_08_grass7/ --exec python -c "from grass.pygrass.modules import Module; Module('g.region', rast='elevation')"
grass --tmp-mapset ~/grassdata/nc_spm_08_grass7/ --exec python -c "from grass.pygrass.modules import Module; Module('v.db.select', map='hospitals', nv='N/A')"
grass --tmp-mapset ~/grassdata/nc_spm_08_grass7/ --exec python -c "from grass.pygrass.modules import Module; Module('v.to.rast', input='lakes', output='lakes', use='val')"

Expected behavior

grass.pygrass.modules.Module should follow the parser rules. This will allow us to introduce changes in backward compatible manner between major version and will allow users to benefit from parser's backward compatibility features for major releases (even when limit ourselves to making changes only for major releases).

Using the parser to do the validation seems like a good solution to me as it relies on a single source of truth about validity of the interface. This would mean skipping the validation in Python completely (like run_command does) or calling the tool itself to check the parameters (like --json but with different error handling).

Screenshots

grass.exceptions.ParameterError: rast is not a valid parameter.
grass.exceptions.ParameterError: nv is not a valid parameter.
ValueError: The Parameter <use>, must be one of the following values: ['attr', 'cat', 'value', 'z', 'dir']

System description

• GRASS GIS version: all, but especially the main branch

Additional context

Backward compatibility issue was created in #3110. Renaming most recently came up in #2189.

[wenzeslaus]

Here is where I'm at right now with teaching pygrass about the backwards compatibility supported by the command line interface parser: I can use return code (0 or 1) of --json to decide if the parameters are valid. However, I don't know how to circumvent pygrass parameter setting mechanism which includes the strict checking. Also, using the tool (e.g., with --json) checks all parameters and their values at once while pygrass does the parameters one by one.

    def update(self, *args, **kargs):
        ...
        # Pseudo-code
        for parameter in parameters:
            if (parameter known to pygrass):
                set_the_parameter()
            else:
                # Before marking parameter as invalid, ask the underlying tool.
                if (not self._ask_if_parameters_are_valid()):
                    raise ParameterError("%s is not a valid parameter." % key)
                # If valid, exception is not raised, but parameter is not set anyway.

    def _ask_if_parameters_are_valid(self):
        """Ask the underlying tool whether the parameters are valid or not"""
        cmd = self.make_cmd()
        cmd.append("--json")
        self.start_time = time.time()
        process = subprocess.run(
            cmd,
            capture_output=True,
            env=self.env_,
        )
        return process.returncode == 0

[wenzeslaus]

I can make it work by just removing all the parameter checking from pygrass. I'm wondering if people make use of that or not.

As a reminder, run_command family of functions from grass.script is relying on the underlying tool to report any issues with parameters. While this is not disabled by grass.pygrass, the Module class from grass.pygrass has its own layer of checks before it runs the tool, so you get grass.exceptions.ParameterError or standard ValueError for the individual parameter name or value.