Repository: omni-us/jsonargparse
Pull request #218 (2022-12-19 07:23:24)

Link arguments error improvements
## What does this PR do?

- Added specific error message for when an argument link uses as source the target of a previous link and vice versa (#208).
- Clearer error message for when an argument link targets a subclass and the target key does not have init_args (pytorch-lightning#16032).

Fixes https://github.com/Lightning-AI/lightning/issues/16032

## Before submitting

- [x] Did you read the [contributing guideline](https://github.com/omni-us/jsonargparse/blob/master/CONTRIBUTING.rst)?
- [n/a] Did you update **the documentation**? (readme and public docstrings)
- [x] Did you write **unit tests** such that there is 100% coverage on related code? (required for bug fixes and new features)
- [x] Did you verify that new and existing **tests pass locally**?
- [x] Did you make sure that all changes preserve **backward compatibility**?
- [x] Did you update **the CHANGELOG**? (not for typos, docs, test updates, or minor internal changes/refactors)


sonarcloud[bot]: Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=omni-us_jsonargparse&pullRequest=218)

[![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=BUG)  
[![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=VULNERABILITY)  
[![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=SECURITY_HOTSPOT)  
[![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=omni-us_jsonargparse&pullRequest=218&resolved=false&types=CODE_SMELL)

[![No Coverage information](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/NoCoverageInfo-16px.png 'No Coverage information')](https://sonarcloud.io/component_measures?id=omni-us_jsonargparse&pullRequest=218) No Coverage information  
[![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=omni-us_jsonargparse&pullRequest=218&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=omni-us_jsonargparse&pullRequest=218&metric=new_duplicated_lines_density&view=list)



Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index aef6d92..8885520 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -22,6 +22,9 @@ Added
   Base64.
 - Support getting the import path of variables in modules, e.g.
   ``random.randint``.
+- Specific error messages for when an argument link uses as source the target of
+  a previous parse link and vice versa `#208
+  <https://github.com/omni-us/jsonargparse/issues/208>`__.
 
 Fixed
 ^^^^^
@@ -32,6 +35,12 @@ Fixed
   <https://github.com/omni-us/jsonargparse/issues/205>`__.
 - Issues reported by CodeQL.
 
+Changed
+^^^^^^^
+- Clearer error message for when an argument link targets a subclass and the
+  target key does not have ``init_args`` `pytorch-lightning#16032
+  <https://github.com/Lightning-AI/lightning/issues/16032>`__.
+
 
 v4.18.0 (2022-11-29)
 --------------------
diff --git a/jsonargparse/link_arguments.py b/jsonargparse/link_arguments.py
index b44d480..0329aab 100644
--- a/jsonargparse/link_arguments.py
+++ b/jsonargparse/link_arguments.py
@@ -109,17 +109,12 @@ class ActionLink(Action):
         compute_fn: Optional[Callable] = None,
         apply_on: str = 'parse',
     ):
+        if not hasattr(parser, '_links_group'):
+            parser._links_group = parser.add_argument_group('Linked arguments')
         self.parser = parser
-
-        # Set and check apply_on
         self.apply_on = apply_on
-        if apply_on not in {'parse', 'instantiate'}:
-            raise ValueError("apply_on must be 'parse' or 'instantiate'.")
-
-        # Set and check compute function
         self.compute_fn = compute_fn
-        if compute_fn is None and not isinstance(source, str):
-            raise ValueError('Multiple source keys requires a compute function.')
+        self._initial_input_checks(source, target)
 
         # Set and check source actions or group
         exclude = (ActionLink, _ActionConfigLoad, _ActionSubCommands, ActionConfigFile)
@@ -143,8 +138,9 @@ class ActionLink(Action):
         is_target_subclass = ActionTypeHint.is_subclass_typehint(self.target[1])
         valid_target_init_arg = is_target_subclass and target.startswith(self.target[1].dest+'.init_args.')
         valid_target_leaf = self.target[1].dest == target
-        if not (valid_target_leaf or valid_target_init_arg):
-            raise ValueError(f'Target key "{target}" must be for an individual argument.')
+        if not valid_target_leaf and is_target_subclass and not valid_target_init_arg:
+            prefix = self.target[1].dest+'.init_args.'
+            raise ValueError(f'Target key expected to start with "{prefix}", got "{target}".')
 
         # Replace target action with link action
         if not is_target_subclass or valid_target_leaf:
@@ -169,8 +165,6 @@ class ActionLink(Action):
             sub_add_kwargs['linked_targets'].add(subtarget)
 
         # Add link action to group to show in help
-        if not hasattr(parser, '_links_group'):
-            parser._links_group = parser.add_argument_group('Linked arguments')
         parser._links_group._group_actions.append(self)
 
         # Check instantiation link does not create cycle
@@ -204,6 +198,31 @@ class ActionLink(Action):
             help=help_str,
         )
 
+    def _initial_input_checks(self, source, target):
+        # Check apply_on
+        if self.apply_on not in {'parse', 'instantiate'}:
+            raise ValueError("apply_on must be 'parse' or 'instantiate'.")
+
+        # Check compute function
+        if self.compute_fn is None and not isinstance(source, str):
+            raise ValueError('Multiple source keys requires a compute function.')
+
+        if self.apply_on == 'parse':
+            # Check source
+            link_actions = self.parser._links_group._group_actions
+            existing_targets = {a.target[0] for a in link_actions}
+            for src in [source] if isinstance(source, str) else source:
+                if src in existing_targets:
+                    raise ValueError(f'Source "{src}" not allowed since it is the target of another link.')
+            # Check target
+            existing_sources = {
+                s[0]
+                for a in link_actions
+                for s in a.source if a.apply_on == 'parse'
+            }
+            if target in existing_sources:
+                raise ValueError(f'Target "{target}" not allowed since it is the source of another link.')
+
     def __call__(self, *args, **kwargs):
         source = ', '.join(s[0] for s in self.source)
         raise TypeError(f'Linked "{self.target[0]}" must be given via "{source}".')


Test patch:
diff --git a/jsonargparse_tests/test_link_arguments.py b/jsonargparse_tests/test_link_arguments.py
index 9a71eee..1c69b25 100755
--- a/jsonargparse_tests/test_link_arguments.py
+++ b/jsonargparse_tests/test_link_arguments.py
@@ -22,6 +22,28 @@ from jsonargparse_tests.base import TempDirTestCase, mock_module
 
 class LinkArgumentsTests(unittest.TestCase):
 
+    def test_link_arguments_previous_target_as_source_error(self):
+        parser = ArgumentParser()
+        parser.add_argument('--a')
+        parser.add_argument('--b')
+        parser.add_argument('--c')
+        parser.link_arguments('a', 'b')
+        with self.assertRaises(ValueError) as ctx:
+            parser.link_arguments('b', 'c')
+        self.assertIn('Source "b" not allowed', str(ctx.exception))
+
+
+    def test_link_arguments_previous_source_as_target_error(self):
+        parser = ArgumentParser()
+        parser.add_argument('--a')
+        parser.add_argument('--b')
+        parser.add_argument('--c')
+        parser.link_arguments('b', 'c')
+        with self.assertRaises(ValueError) as ctx:
+            parser.link_arguments('a', 'b')
+        self.assertIn('Target "b" not allowed', str(ctx.exception))
+
+
     def test_link_arguments_on_parse_compute_fn_single_arguments(self):
         parser = ArgumentParser(error_handler=None)
         parser.add_argument('--a.v1', default=2)
@@ -339,8 +361,9 @@ class LinkArgumentsTests(unittest.TestCase):
 
         # Source must be subclass action or class group
         parser = make_parser_2()
-        with self.assertRaises(ValueError):
+        with self.assertRaises(ValueError) as ctx:
             parser.link_arguments('a.b.c', 'b.b3', apply_on='instantiate')
+        self.assertIn('Target key expected to start with "b.init_args."', str(ctx.exception))
 
 
     def test_link_arguments_on_instantiate_no_compute_no_target_instantiate(self):