Repository: skypyproject/skypy

Issue:
unexpected keyword argument '.depends'
**Describe the bug**
There are situations (I can't narrow down the exact conditions) where the `Call` class is passing `.depends` as a keyword argument to its function call (when run within a Pipeline). I am working on a fix.

**Minimum working example**
```yml
# bug.yml
tables:
  bug_table_1:
    a: !numpy.random.uniform [0, 1, 10]
  bug_table_2:
    .init: !astropy.table.vstack
      tables: [ $bug_table_1 ]
      .depends: [ bug_table_1.complete ]
```

```bash
>>> skypy -v bug.yml tmp.fits
2021-05-12 16:04:51,626 [ERROR] skypy: got an unexpected keyword argument '.depends'
Traceback (most recent call last):
  File "skypy/pipeline/scripts/skypy.py", line 42, in main
    pipeline = Pipeline(config)
  File "skypy/pipeline/_pipeline.py", line 106, in __init__
    settings.infer(context)
  File "skypy/pipeline/_items.py", line 66, in infer
    given = sig.bind_partial(*self.args, **self.kwargs)
  File "/usr/local/opt/python@3.8/Frameworks/Python.framework/Versions/3.8/lib/python3.8/inspect.py", line 3044, in bind_partial
    return self._bind(args, kwargs, partial=True)
  File "/usr/local/opt/python@3.8/Frameworks/Python.framework/Versions/3.8/lib/python3.8/inspect.py", line 3026, in _bind
    raise TypeError(
TypeError: got an unexpected keyword argument '.depends'
```


Discussion:
rrjbca: Two further refactors I have considered. Thoughts?
- Change the config syntax for explicit dependencies to `.depends: $dependency_name` i.e. a `Ref` instead of a string
- Change `Item.depend()` to return a list of `Ref` objects instead of a list of strings
ntessore: This moves the dependency structure exactly the opposite way of what I had in mind originally: I thought the `Pipeline` should eventually be the only entity that knows about dependencies. And the current implementation was just a half way hack to get there from how it was before.

Not that I know a better way to handle it than what is proposed here, mind. Just to add some context to why the current system is what it is.
rrjbca: Now that the PR handles dependencies all in one place (`Item`) rather than mixed between two (`Item` & `Pipeline`) I imagine it is possible to move the entire implementation into `Pipeline` if that makes more sense? I'm happy to try and make that change here.
ntessore: > I imagine it is possible to move the entire implementation into `Pipeline` if that makes more sense?

To me it does make more sense, because an `Item` does not know about the existence of other items, whereas the `Pipeline` knows about the existence of all the dependent objects.

> I'm happy to try and make that change here.

If you want to, I think it would be a fantastic simplification, but IIRC it's potentially hard to do the dependency resolution of arguments to `Call`s in `Pipeline`. I would not make that a requirement for accepting the PR if it turns out to be challenging.
rrjbca: I have pushed a more minimal fix, @ntessore I assume you prefer this? It should also be possible to refactor such that `Call.depend` and `Pipeline.depend` don't recursively call each other which I'll think some more about. That isn't needed for this PR though and if approved I'm happy to merge as is.
ntessore: Since the `context` for `cosmology` is merely a `$cosmology` reference, does the current solution actually add a dependency between the function call and the cosmology? That's what happened before and why the context resolution was in such an awkward place.

On the other hand, since the `context` mechanism was planned to add something like scope of a variable, perhaps context resolution should not introduce additional dependency by design?
rrjbca: > Since the `context` for `cosmology` is merely a `$cosmology` reference, does the current solution actually add a dependency between the function call and the cosmology?
> 

From looking at the code no I don't think so, but cosmology is explicitly evaluated before anything else when calling `Pipeline.evaluate` so technically a dependency isn't required. However this isn't the case for any other context variables which is an issue.
rrjbca: @skypyproject/skypy-infrastructure could somebody please review (and approve) this fix? I will open a separate issue for the missing context dependencies identified by @ntessore above.

Patch:
diff --git a/skypy/pipeline/_pipeline.py b/skypy/pipeline/_pipeline.py
index 474ed1c..84be0a6 100644
--- a/skypy/pipeline/_pipeline.py
+++ b/skypy/pipeline/_pipeline.py
@@ -102,8 +102,6 @@ class Pipeline:
             self.dag.add_node(job, skip=False)
             if isinstance(settings, Item):
                 items[job] = settings
-                # infer additional item properties from context
-                settings.infer(context)
         for table, columns in self.table_config.items():
             table_complete = '.'.join((table, 'complete'))
             self.dag.add_node(table_complete)
@@ -115,8 +113,6 @@ class Pipeline:
                 self.dag.add_edge(job, table_complete)
                 if isinstance(settings, Item):
                     items[job] = settings
-                    # infer additional item properties from context
-                    settings.infer(context)
                 # DAG nodes for individual columns in multi-column assignment
                 names = [n.strip() for n in column.split(',')]
                 if len(names) > 1:
@@ -138,6 +134,8 @@ class Pipeline:
                 while c:
                     self.dag.add_edge(c, d)
                     c, d = c.rpartition('.')[0], c
+            # infer additional item properties from context
+            settings.infer(context)
 
     def execute(self, parameters={}):
         r'''Run a pipeline.


Test patch:
diff --git a/skypy/pipeline/tests/test_pipeline.py b/skypy/pipeline/tests/test_pipeline.py
index cd42ba5..3b6151f 100644
--- a/skypy/pipeline/tests/test_pipeline.py
+++ b/skypy/pipeline/tests/test_pipeline.py
@@ -2,7 +2,7 @@ from astropy.cosmology import FlatLambdaCDM, default_cosmology
 from astropy.cosmology.core import Cosmology
 from astropy.io import fits
 from astropy.io.misc.hdf5 import read_table_hdf5
-from astropy.table import Table
+from astropy.table import Table, vstack
 from astropy.table.column import Column
 from astropy.units import Quantity
 from astropy.utils.data import get_pkg_data_filename
@@ -267,3 +267,25 @@ def test_hdf5(tmp_path):
     pipeline.write(output_filename)
     hdf_table = read_table_hdf5(output_filename, 'tables/test_table', character_as_bytes=False)
     assert np.all(hdf_table == pipeline['test_table'])
+
+
+def test_depends():
+
+    # Regression test for GitHub Issue #464
+    # Previously the .depends keyword was also being passed to functions as a
+    # keyword argument. This was because Pipeline was executing Item.infer to
+    # handle additional function arguments from context before handling
+    # additional dependencies specified using the .depends keyword. The
+    # .depends keyword is now handled first.
+
+    config = {'tables': {
+                'table_1': {
+                  'column1': Call(np.random.uniform, [0, 1, 10])},
+                'table_2': {
+                    '.init': Call(vstack, [], {
+                      'tables': [Ref('table_1')],
+                      '.depends': ['table_1.complete']})}}}
+
+    pipeline = Pipeline(config)
+    pipeline.execute()
+    assert np.all(pipeline['table_1'] == pipeline['table_2'])