Repository: AgnostiqHQ/covalent
Pull request #1593 (2023-04-24 04:08:25)

raise error on extra args/kwargs
<!--
⚠️ Your pull request title should be short detailed and understandable for all.
⚠️ Increment the version number in the /VERSION file. If this is a bugfix, increment the patch version (the rightmost number), for example 0.18.2 becomes 0.18.3. If it's a new feature, increment the minor version (the middle number), for example 0.18.2 becomes 0.19.0.
⚠️ Add a note to /CHANGELOG.md with your version number and the date, summarizing the changes.
⚠️ If your pull request fixes an open issue, please link to the issue.
⚠️ Rebase the latest changes from the develop branch. Assuming origin points to https://github.com/AgnostiqHQ/covalent, you should run git rebase origin/develop.
-->

Fixes https://github.com/AgnostiqHQ/covalent/issues/1584

- [x] I have added the tests to cover my changes.
- [x] I have updated the documentation and CHANGELOG accordingly.
- [x] I have read the CONTRIBUTING document.


codecov[bot]: ## [Codecov](https://codecov.io/gh/AgnostiqHQ/covalent/pull/1593?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ) Report
> Merging [#1593](https://codecov.io/gh/AgnostiqHQ/covalent/pull/1593?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ) (f9b4ad8) into [develop](https://codecov.io/gh/AgnostiqHQ/covalent/commit/9ad4311d342afdf97a1668e81244e815916d7ea3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ) (9ad4311) will **increase** coverage by `0.85%`.
> The diff coverage is `75.00%`.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://codecov.io/gh/AgnostiqHQ/covalent/pull/1593/graphs/tree.svg?width=650&height=150&src=pr&token=YGHCB3DE4P&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ)](https://codecov.io/gh/AgnostiqHQ/covalent/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AgnostiqHQ)

```diff
@@             Coverage Diff             @@
##           develop    #1593      +/-   ##
===========================================
+ Coverage    85.90%   86.76%   +0.85%     
===========================================
  Files          159       83      -76     
  Lines         6641     4955    -1686     
  Branches       145        0     -145     
===========================================
- Hits          5705     4299    -1406     
+ Misses         834      656     -178     
+ Partials       102        0     -102     
```

| Flag | Coverage Δ | |
|---|---|---|
| Dispatcher | `87.22% <ø> (-0.06%)` | :arrow_down: |
| Functional_Tests | `?` | |
| SDK | `86.47% <75.00%> (-0.05%)` | :arrow_down: |
| UI_Frontend | `?` | |

</details>
madhur-tandon: > It might be worthwhile to try a more robust check with `inspect.signature` and let user know specifically which arguments or kwargs are missing or extra in the error as well.

So, I already tried `getfullargspec` stuff. There are two issues:

**1. It doesn't preserve the original function signature, probably because decorators change the metadata...**

Eg, if you try:

```
import covalent as ct

@ct.lattice
@ct.electron
def job(val1, val2):
    return val1 + val2
```

and do a `getfullargspec` on `func` inside the `get_named_params` function, we get:

```
FullArgSpec(args=[], varargs='args', varkw='kwargs', defaults=None, kwonlyargs=[],
kwonlydefaults=None, annotations={})
```

Clearly, the information about `val1` and `val2` is lost... while `func` still refers to `<function job at 0x106ae8b80>`

**2. It interferes with postprocessing...**

Specifically, `build_graph` calls `pp.add_exhaustive_postprocess_node` at some point which further uses the `_postprocess` function which has the signature `def _postprocess(self, *ordered_node_outputs: List[Any]) -> Any:`

Doing a `getfullargspec` on this gives us:

```
FullArgSpec(args=['self'], varargs='ordered_node_outputs', varkw=None, defaults=None, kwonlyargs=[],
kwonlydefaults=None, annotations={'return': typing.Any, 'ordered_node_outputs': typing.List[typing.Any]})
```

As one can see, we cannot use this information to count our `varargs` in some sense. Plus, it also includes `self` as one of the arguments (and will probably include `cls` too for class methods, if any).

Thus, even without using `getfullargspec` and directly counting with
`ordered_params_dict = inspect.signature(func).parameters` also leads to issues i.e. a simple comparison such as
`if len(args) + len(kwargs) > len(ordered_params_dict)` aka my first commit here: https://github.com/AgnostiqHQ/covalent/pull/1593/commits/79610c283c6b7e3fa81dbcbf6cace234d9e27752 will lead to errors...

---

In fact, the current approach of iterating and figuring out the `named_args` and `named_kwargs` is much better and hence, I resorted to using that itself...

My suggestion would be to tackle this "extended" functionality as a separate issue / PR owing to the above problems + the original issue also had a limited scope anyway...
kessler-frost: Hey @madhur-tandon thanks for the PR 😄 ! I agree with the points you mentioned and that the current method of determining named args and kwargs is better, but I think what @santoshkumarradha mentioned:

> let user know specifically which arguments or kwargs are missing or extra in the error as well.

is quite achievable by only a few minor changes to the modifications you already made in the PR. For example, in the case of positional arguments we can tell the user how many extra args they have passed vs how many were expected. And for the keyword arguments we can tell them what were the keys that they passed which were not expected in the raised `ValueError`. Basically instead of comparing the length of all parameters, we split that into 2 `if` conditions one for `args` and one for `kwargs`.

Again, I realize the scope of the original issue did not include improvement in the error description, but the changes required to add that description shouldn't be a lot.

PS:
- You can use `set` for the key comparison: `set(kwargs.keys()) - set(named_kwargs.keys())`
- Bonus points if you can also handle the case where fewer arguments are passed than required. Something like this should help:
```python
        if param.default is param.empty and param_name not in kwargs and ind >= len(args):
            raise ValueError(f"Parameter {param_name} is required, but not supplied.")
```
madhur-tandon: @kessler-frost Sure, I TOO think that some improvements can be made with the current mechanism of figuring out the `named_args` and `named_kwargs`, I wrote the above text to:

- specifically highlight the issues with `getfullargspec` and that I had already tried it
- suggest that we could do the `extended` functionality in another PR

But, now that I think of it again, probably adding the same in this PR makes sense as well.

So, thanks for your comments, I will probably make some changes, add tests, and push again in some time...
kessler-frost: Sounds good 🤘
madhur-tandon: I suggest not to figure out the case where fewer arguments are present and the associated check of:

```
if param.default is param.empty and param_name not in kwargs and ind >= len(args):
    raise ValueError(f"Parameter {param_name} is required, but not supplied.")
```

Instead, since we call the function anyway at some point i.e. inside `build_graph`:

```
try:
    retval = workflow_function(*new_args, **new_kwargs)
```

The above call is enough to determine missing arguments and will throw an error on it's own i.e. it won't be thrown by us, but the purpose is served anyway.

I say because, considering the below snippet for example:

```
@ct.electron
def task(x, y, *z):
    return x + y + sum(z)

@ct.lattice
def workflow(x, y, *z):
    return task(x, y, *z)

workflow.build_graph(x=7, y=8)
```

There are no issues with this but then, the snippet for calculating fewer arguments would check for `z` here as well.

Thus, it is better to outsource this to the call of the function instead.
kessler-frost: Sure, we can figure out the resolution for that in a separate issue then.
madhur-tandon: @kessler-frost seems like there's an issue with CI and resolving the environment and dependencies, can you have a look and re-trigger the CI?

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f9afb03a..87dc6045 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Result status comparison
+- Raise error on extra args/kwargs
 
 ## [0.221.0-rc.0] - 2023-04-17
 
diff --git a/covalent/_shared_files/utils.py b/covalent/_shared_files/utils.py
index 1d0ff33b..9c773265 100644
--- a/covalent/_shared_files/utils.py
+++ b/covalent/_shared_files/utils.py
@@ -180,6 +180,7 @@ def required_params_passed(func: Callable, kwargs: Dict) -> bool:
 
 def get_named_params(func, args, kwargs):
     ordered_params_dict = inspect.signature(func).parameters
+
     named_args = {}
     named_kwargs = {}
 
@@ -194,12 +195,20 @@ def get_named_params(func, args, kwargs):
         elif param.kind == param.VAR_POSITIONAL:
             for i in range(ind, len(args)):
                 named_args[f"arg[{i}]"] = args[i]
-
         elif param.kind in [param.KEYWORD_ONLY, param.VAR_KEYWORD]:
             for key, value in kwargs.items():
                 if key != param_name:
                     named_kwargs[key] = value
 
+    if len(args) > len(named_args):
+        raise ValueError(
+            f"Too many positional arguments given, expected {len(named_args)}, received {len(args)}"
+        )
+
+    if len(kwargs) > len(named_kwargs):
+        extra_supplied_kwargs = ", ".join(sorted(set(kwargs.keys()) - set(named_kwargs.keys())))
+        raise ValueError(f"Unexpected keyword arguments: {extra_supplied_kwargs}")
+
     return (named_args, named_kwargs)
 
 


Test patch:
diff --git a/tests/covalent_dispatcher_tests/_cli/migrate_test.py b/tests/covalent_dispatcher_tests/_cli/migrate_test.py
index 1d010684..86975937 100644
--- a/tests/covalent_dispatcher_tests/_cli/migrate_test.py
+++ b/tests/covalent_dispatcher_tests/_cli/migrate_test.py
@@ -237,9 +237,6 @@ def test_process_lattice():
     assert lattice.metadata["call_after"] == []
 
 
-2
-
-
 def test_process_result_object():
     """Test process_result_object"""
 
diff --git a/tests/covalent_tests/workflow/lattice_test.py b/tests/covalent_tests/workflow/lattice_test.py
index d4424f0f..b4eb3377 100644
--- a/tests/covalent_tests/workflow/lattice_test.py
+++ b/tests/covalent_tests/workflow/lattice_test.py
@@ -18,10 +18,12 @@
 #
 # Relief from the License may be granted by purchasing a commercial license.
 
-"""Unit tests for electron"""
+"""Unit tests for lattice"""
 
 from dataclasses import asdict
 
+import pytest
+
 import covalent as ct
 from covalent._shared_files.defaults import DefaultMetadataValues, postprocess_prefix
 
@@ -107,3 +109,38 @@ def test_lattice_build_graph(mocker):
     workflow.build_graph(1)
     assert workflow.transport_graph.get_node_value(2, "name") == postprocess_prefix
     ct._shared_files.config.set_config("sdk.exhaustive_postprocess", original_exhaustive_value)
+
+
+def test_lattice_build_graph_with_extra_args(mocker):
+    """Test the build graph method in lattice with extra args / kwargs."""
+
+    @ct.electron
+    def task(x, y):
+        return x + y
+
+    @ct.lattice
+    def workflow(x, y):
+        return task(x, y)
+
+    with pytest.raises(
+        ValueError, match="Too many positional arguments given, expected 2, received 3"
+    ):
+        workflow.build_graph(1, 2, 3)
+
+    with pytest.raises(
+        ValueError, match="Too many positional arguments given, expected 0, received 1"
+    ):
+        workflow.build_graph(1, x=2)
+
+    # no issues here
+    workflow.build_graph(1, y=2)
+
+    with pytest.raises(ValueError, match="Unexpected keyword arguments: a, b"):
+        workflow.build_graph(1, a=2, b=3)
+
+    with pytest.raises(ValueError, match="Unexpected keyword arguments: a"):
+        workflow.build_graph(a=1)
+
+    # fewer arguments handled internally by function call
+    with pytest.raises(TypeError, match="missing 1 required positional argument: 'y'"):
+        workflow.build_graph(1)