BUG: Move handling of context arguments after handling of .depends keyword

[rrjbca]

Description

Pipeline constructor now correctly infers additional arguments for Call objects after handling their extra explicit dependencies specified using the .depends keyword. Resolves #464.

Checklist

• Follow the Contributor Guidelines
• Write unit tests
• Write documentation strings
• Assign someone from your working team to review this pull request
• Assign someone from the infrastructure team to review this pull request

[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 Calls 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.

Currently cosmology is hardcoded as the only context variable, and it is also hardcoded to be evaluated before all other DAG jobs. Context variable dependencies in the DAG is therefore not currently an issue, but will need to be addressed when new context variables are added in the future.

[philipp128]

Looks goof to me. Tests have passed for this.

[philipp128]

Merging conflict solved. Rest as before when I approved.