Repository: OCR-D/core (python)

Issue:
parameter decorator: handle preset files as resources
When we drafted the resource handling, one of the use cases was installing preset files, i.e. preconfigured (if incomplete) json parameter sets that have a discernable name and can be used as value for `--parameter|-p` but without the directory path.

That latter part is still missing. So we still need to modify … https://github.com/OCR-D/core/blob/0713e0e1c2fd11242957c3a51c974a1048dba997/ocrd/ocrd/decorators/parameter_option.py#L7-L14 … such that known resource filenames are tried first.

Pull request description:
parameter decorator: resolve path names as resources
Fixes #930

Replaces #969 


Patch:
diff --git a/src/ocrd/cli/ocrd_tool.py b/src/ocrd/cli/ocrd_tool.py
index 4d098375..2a7fa99e 100644
--- a/src/ocrd/cli/ocrd_tool.py
+++ b/src/ocrd/cli/ocrd_tool.py
@@ -18,6 +18,7 @@ from ocrd.processor import Processor
 from ocrd_utils import (
     set_json_key_value_overrides,
     VERSION as OCRD_VERSION,
+    parse_json_string_or_file,
     parse_json_string_with_comments as loads
 )
 from ocrd_validators import ParameterValidator, OcrdToolValidator
@@ -104,6 +105,17 @@ def ocrd_tool_tool_list_resources(ctx):
     BashProcessor(None, ocrd_tool=ctx.json['tools'][ctx.tool_name],
                   list_resources=True)
 
+@ocrd_tool_tool.command('resolve-resource', help="Get a tool's file resource full path name")
+@click.argument('res_name')
+@pass_ocrd_tool
+def ocrd_tool_tool_resolve_resource(ctx, res_name):
+    class BashProcessor(Processor):
+        @property
+        def moduledir(self):
+            return os.path.dirname(ctx.filename)
+    BashProcessor(None, ocrd_tool=ctx.json['tools'][ctx.tool_name],
+                  resolve_resource=res_name)
+
 @ocrd_tool_tool.command('show-resource', help="Dump a tool's file resource")
 @click.argument('res_name')
 @pass_ocrd_tool
@@ -169,7 +181,7 @@ def ocrd_tool_tool_parse_params(ctx, parameter, parameter_override, json):
     """
     Parse parameters with fallback to defaults and output as shell-eval'able assignments to params var.
     """
-    set_json_key_value_overrides(parameter, *parameter_override)
+    parameter = set_json_key_value_overrides(parse_json_string_or_file(*parameter), *parameter_override)
     parameterValidator = ParameterValidator(ctx.json['tools'][ctx.tool_name])
     report = parameterValidator.validate(parameter)
     if not report.is_valid:
diff --git a/src/ocrd/decorators/__init__.py b/src/ocrd/decorators/__init__.py
index 4e46dec0..811587a1 100644
--- a/src/ocrd/decorators/__init__.py
+++ b/src/ocrd/decorators/__init__.py
@@ -8,12 +8,13 @@ from ocrd_utils import (
     getLogger,
     parse_json_string_with_comments,
     set_json_key_value_overrides,
+    parse_json_string_or_file,
 )
 from ocrd_validators import WorkspaceValidator
 from ocrd_network import ProcessingWorker, ProcessorServer, AgentType
 
 from ..resolver import Resolver
-from ..processor.base import run_processor
+from ..processor.base import ResourceNotFoundError, run_processor
 
 from .loglevel_option import ocrd_loglevel
 from .parameter_option import parameter_option, parameter_override_option
@@ -71,6 +72,18 @@ def ocrd_cli_wrap_processor(
 
     LOG = getLogger('ocrd.cli_wrap_processor')
     # LOG.info('kwargs=%s' % kwargs)
+    if 'parameter' in kwargs:
+        # Disambiguate parameter file/literal, and resolve file
+        disposable = processorClass(workspace=None)
+        def resolve(name):
+            try:
+                return disposable.resolve_resource(name)
+            except ResourceNotFoundError:
+                return None
+        kwargs['parameter'] = parse_json_string_or_file(*kwargs['parameter'],
+                                                        resolve_preset_file=resolve)
+    else:
+        kwargs['parameter'] = dict()
     # Merge parameter overrides and parameters
     if 'parameter_override' in kwargs:
         set_json_key_value_overrides(kwargs['parameter'], *kwargs['parameter_override'])
diff --git a/src/ocrd/decorators/parameter_option.py b/src/ocrd/decorators/parameter_option.py
index 7fb989be..0fbe3e05 100644
--- a/src/ocrd/decorators/parameter_option.py
+++ b/src/ocrd/decorators/parameter_option.py
@@ -1,5 +1,5 @@
 from click import option
-from ocrd_utils import parse_json_string_or_file
+#from ocrd_utils import parse_json_string_or_file
 
 __all__ = ['parameter_option', 'parameter_override_option']
 
@@ -11,7 +11,9 @@ parameter_option = option('-p', '--parameter',
                                 help="Parameters, either JSON string or path to JSON file",
                                 multiple=True,
                                 default=['{}'],
-                                callback=_handle_param_option)
+                                # now handled in ocrd_cli_wrap_processor to resolve processor preset files
+                                # callback=_handle_param_option
+                                callback=lambda ctx, param, kv: list(kv))
 
 parameter_override_option = option('-P', '--parameter-override',
                                 help="Parameter override",
diff --git a/src/ocrd/lib.bash b/src/ocrd/lib.bash
index b3067d02..a65ca35c 100644
--- a/src/ocrd/lib.bash
+++ b/src/ocrd/lib.bash
@@ -67,6 +67,13 @@ ocrd__dumpjson () {
     ocrd ocrd-tool "$OCRD_TOOL_JSON" tool "$OCRD_TOOL_NAME" dump
 }
 
+##
+## Output file resource path.
+##
+ocrd__resolve_resource () {
+    ocrd ocrd-tool "$OCRD_TOOL_JSON" tool "$OCRD_TOOL_NAME" resolve-resource "$1"
+}
+
 ##
 ## Output file resource content.
 ##
@@ -139,7 +146,7 @@ ocrd__parse_argv () {
             -D|--dump-module-dir) echo $(dirname "$OCRD_TOOL_JSON"); exit ;;
             -C|--show-resource) ocrd__show_resource "$2"; exit ;;
             -L|--list-resources) ocrd__list_resources; exit ;;
-            -p|--parameter) __parameters+=(-p "$2") ; shift ;;
+            -p|--parameter)  __parameters+=(-p $(ocrd__resolve_resource "$2" 2>/dev/null || echo "$2")) ; shift ;;
             -P|--parameter-override) __parameter_overrides+=(-P "$2" "$3") ; shift ; shift ;;
             -g|--page-id) ocrd__argv[page_id]=$2 ; shift ;;
             -O|--output-file-grp) ocrd__argv[output_file_grp]=$2 ; shift ;;
diff --git a/src/ocrd/processor/__init__.py b/src/ocrd/processor/__init__.py
index f01e2b3c..21b0c69e 100644
--- a/src/ocrd/processor/__init__.py
+++ b/src/ocrd/processor/__init__.py
@@ -1,5 +1,6 @@
 from .base import (
     Processor,
+    ResourceNotFoundError
 )
 from .helpers import (
     run_cli,
diff --git a/src/ocrd/processor/base.py b/src/ocrd/processor/base.py
index 3df7d8c1..931d945d 100644
--- a/src/ocrd/processor/base.py
+++ b/src/ocrd/processor/base.py
@@ -38,6 +38,19 @@ from ocrd_models.ocrd_page import MetadataItemType, LabelType, LabelsType
 # XXX imports must remain for backwards-compatibility
 from .helpers import run_cli, run_processor, generate_processor_help # pylint: disable=unused-import
 
+class ResourceNotFoundError(FileNotFoundError):
+    """
+    An exception signifying the requested processor resource
+    cannot be resolved.
+    """
+    def __init__(self, name, executable):
+        self.name = name
+        self.executable = executable
+        self.message = "Could not find resource '%s' for executable '%s'. " \
+                       "Try 'ocrd resmgr download %s %s' to download this resource." \
+                       % (name, executable, executable, name)
+        super().__init__(self.message)
+
 class Processor():
     """
     A processor is a tool that implements the uniform OCR-D command-line interface
@@ -59,6 +72,7 @@ class Processor():
             input_file_grp="INPUT",
             output_file_grp="OUTPUT",
             page_id=None,
+            resolve_resource=None,
             show_resource=None,
             list_resources=False,
             show_help=False,
@@ -87,6 +101,8 @@ class Processor():
              output_file_grp (string): comma-separated list of METS ``fileGrp``s used for output.
              page_id (string): comma-separated list of METS physical ``page`` IDs to process \
                  (or empty for all pages).
+             resolve_resource (string): If not ``None``, then instead of processing, resolve \
+                 given resource by name and print its full path to stdout.
              show_resource (string): If not ``None``, then instead of processing, resolve \
                  given resource by name and print its contents to stdout.
              list_resources (boolean): If true, then instead of processing, find all installed \
@@ -115,9 +131,17 @@ class Processor():
             for res in self.list_all_resources():
                 print(res)
             return
-        if show_resource:
+        if resolve_resource or show_resource:
             initLogging()
-            res_fname = self.resolve_resource(show_resource)
+            try:
+                res_fname = self.resolve_resource(resolve_resource or show_resource)
+            except ResourceNotFoundError as e:
+                log = getLogger('ocrd.processor.base')
+                log.critical(e.message)
+                sys.exit(1)
+            if resolve_resource:
+                print(res_fname)
+                return
             fpath = Path(res_fname)
             if fpath.is_dir():
                 with pushd_popd(fpath):
@@ -174,7 +198,7 @@ class Processor():
         
         (This contains the main functionality and needs to be overridden by subclasses.)
         """
-        raise Exception("Must be implemented")
+        raise NotImplementedError()
 
 
     def add_metadata(self, pcgts):
@@ -210,7 +234,7 @@ class Processor():
             val (string): resource value to resolve
         """
         executable = self.ocrd_tool['executable']
-        log = getLogger('ocrd.%s.resolve_resource' % executable)
+        log = getLogger('ocrd.processor.base')
         if exists(val):
             log.debug("Resolved to absolute path %s" % val)
             return val
@@ -224,10 +248,7 @@ class Processor():
         if ret:
             log.debug("Resolved %s to absolute path %s" % (val, ret[0]))
             return ret[0]
-        log.error("Could not find resource '%s' for executable '%s'. "
-                  "Try 'ocrd resmgr download %s %s' to download this resource.",
-                  val, executable, executable, val)
-        sys.exit(1)
+        raise ResourceNotFoundError(val, executable)
 
     def list_all_resources(self):
         """
diff --git a/src/ocrd_utils/str.py b/src/ocrd_utils/str.py
index 6a9986ae..51cce4bf 100644
--- a/src/ocrd_utils/str.py
+++ b/src/ocrd_utils/str.py
@@ -159,7 +159,7 @@ def parse_json_string_with_comments(val):
     jsonstr = re.sub(r'^\s*#.*$', '', val, flags=re.MULTILINE)
     return json.loads(jsonstr)
 
-def parse_json_string_or_file(*values):    # pylint: disable=unused-argument
+def parse_json_string_or_file(*values, resolve_preset_file=None):    # pylint: disable=unused-argument
     """
     Parse a string as either the path to a JSON object or a literal JSON object.
 
@@ -173,7 +173,10 @@ def parse_json_string_or_file(*values):    # pylint: disable=unused-argument
             continue
         try:
             try:
-                with open(value, 'r') as f:
+                path = value
+                if callable(resolve_preset_file):
+                    path = resolve_preset_file(value) or value
+                with open(path, 'r') as f:
                     value_parsed = parse_json_string_with_comments(f.read())
             except (FileNotFoundError, OSError):
                 value_parsed = parse_json_string_with_comments(value.strip())


Test patch:
diff --git a/tests/processor/test_processor.py b/tests/processor/test_processor.py
index be4cc56e..784f68fc 100644
--- a/tests/processor/test_processor.py
+++ b/tests/processor/test_processor.py
@@ -1,26 +1,36 @@
 import json
+from contextlib import ExitStack
 
 from tempfile import TemporaryDirectory
-from os.path import join
-from tests.base import CapturingTestCase as TestCase, assets, main # pylint: disable=import-error, no-name-in-module
+from pathlib import Path
+from os import environ
+from tests.base import CapturingTestCase as TestCase, assets, main, copy_of_directory # pylint: disable=import-error, no-name-in-module
 from tests.data import DummyProcessor, DummyProcessorWithRequiredParameters, DummyProcessorWithOutput, IncompleteProcessor
 
 from ocrd_utils import MIMETYPE_PAGE, pushd_popd, initLogging, disableLogging
 from ocrd.resolver import Resolver
 from ocrd.processor.base import Processor, run_processor, run_cli
 
+from unittest import mock
 import pytest
 
 class TestProcessor(TestCase):
 
     def setUp(self):
         super().setUp()
-        self.resolver = Resolver()
-        self.workspace = self.resolver.workspace_from_url(assets.url_of('SBB0000F29300010000/data/mets.xml'))
+        # make sure we get an isolated temporary copy of the testdata each time
+        # as long as we are not using pytest but unittest, we need to manage contexts
+        # (enterContext is only supported starting with py311)
+        with ExitStack() as stack:
+            self.resolver = Resolver()
+            self.workdir = stack.enter_context(copy_of_directory(assets.path_to('SBB0000F29300010000/data')))
+            stack.enter_context(pushd_popd(self.workdir))
+            self.workspace = self.resolver.workspace_from_url('mets.xml')
+            self.addCleanup(stack.pop_all().close)
 
     def test_incomplete_processor(self):
         proc = IncompleteProcessor(None)
-        with self.assertRaisesRegex(Exception, 'Must be implemented'):
+        with self.assertRaises(NotImplementedError):
             proc.process()
 
     def test_no_resolver(self):
@@ -48,8 +58,8 @@ class TestProcessor(TestCase):
         assert [f.mimetype for f in processor.input_files] == [MIMETYPE_PAGE, MIMETYPE_PAGE]
 
     def test_parameter(self):
-        with TemporaryDirectory() as tempdir:
-            jsonpath = join(tempdir, 'params.json')
+        with TemporaryDirectory():
+            jsonpath = Path('params.json').name
             with open(jsonpath, 'w') as f:
                 f.write('{"baz": "quux"}')
             with open(jsonpath, 'r') as f:
@@ -73,6 +83,29 @@ class TestProcessor(TestCase):
         with self.assertRaisesRegex(Exception, 'is a required property'):
             DummyProcessorWithRequiredParameters(workspace=self.workspace)
 
+    def test_params_preset_resolve(self):
+        with pushd_popd(tempdir=True) as tempdir:
+            with mock.patch.dict(environ, {'XDG_DATA_HOME': str(tempdir)}):
+                path = Path(tempdir) / 'ocrd-resources' / 'ocrd-dummy'
+                path.mkdir(parents=True)
+                path = str(path / 'preset.json')
+                with open(path, 'w') as out:
+                    # it would be nicer to test some existing processor which does take params
+                    out.write('{}')
+                assert 0 == run_cli("ocrd-dummy",
+                                    resolver=Resolver(),
+                                    mets_url=self.workspace.mets_target,
+                                    input_file_grp="OCR-D-IMG",
+                                    output_file_grp="DUMMY",
+                                    parameter=path)
+                assert 0 == run_cli("ocrd-dummy",
+                                    resolver=Resolver(),
+                                    mets_url=self.workspace.mets_target,
+                                    input_file_grp="OCR-D-IMG",
+                                    output_file_grp="DUMMY",
+                                    parameter='preset.json',
+                                    overwrite=True)
+
     def test_params(self):
         proc = Processor(workspace=self.workspace)
         self.assertEqual(proc.parameter, {})
diff --git a/tests/test_decorators.py b/tests/test_decorators.py
index 992e173d..3c38f207 100644
--- a/tests/test_decorators.py
+++ b/tests/test_decorators.py
@@ -4,6 +4,7 @@ from contextlib import contextmanager
 from click.testing import CliRunner
 from tempfile import TemporaryDirectory
 from os.path import join, exists
+import pytest
 
 from tests.base import CapturingTestCase as TestCase, assets, main, copy_of_directory # pylint: disable=import-error, no-name-in-module
 from tests.data import DummyProcessor
@@ -105,6 +106,7 @@ class TestDecorators(TestCase):
                 exit_code, out, err = self.invoke_cli(cli_dummy_processor, ['-p', '{"baz": "forty-two"}', '--mets', 'mets.xml', *DEFAULT_IN_OUT])
                 assert not exit_code
 
+    @pytest.mark.skip(reason="now deferred to ocrd_cli_wrap_processor (to resolve preset resources)")
     def test_param_merging(self):
         json1 = '{"foo": 23, "bar": 100}'
         json2 = '{"foo": 42}'