Closed rrjbca closed 3 years ago
Two further refactors I have considered. Thoughts?
.depends: $dependency_name
i.e. a Ref
instead of a stringItem.depend()
to return a list of Ref
objects instead of a list of stringsThis 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.
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.
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.
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.
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?
Since the
context
forcosmology
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.
@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.
Description
Pipeline
constructor now correctly infers additional arguments forCall
objects after handling their extra explicit dependencies specified using the.depends
keyword. Resolves #464.Checklist