Add all_dependencies method to Base Entities#155
Conversation
I agree with this for Would it be alot of work? |
I started prototyping it and it turned into a research project about overloading python setters because ingredient_run has special logic. |
| if self.process is not None: | ||
| result.add(self.process) | ||
|
|
||
| return result |
There was a problem hiding this comment.
This logic feels wrong because it requires base_object to know about the structure of its child classes.
There was a problem hiding this comment.
Yeah, I agree that the isinstance(self, GEMD) would look nicer if we could test mixins instead. No obvious solution for MaterialSpec since PropertyAndConditions is just structured differently than MeasurementRun's property. A HasMaterial and HasProcess mixin would be more straightforward, but it's not clear that it buys you much because the only consequence is that you provide an abstract method that must be implemented where you don't even know the return type.
If necessary, I could bake that chunk of framework out.
There was a problem hiding this comment.
If we can't write this method without having a holistic understanding of the GEMD object structure, then what's the value in having this logic live in BaseObject as opposed to citrine-python?
There was a problem hiding this comment.
Especially since all_dependencies is only used for batching in the context of upload, which is a citrine-python concern
There was a problem hiding this comment.
The mechanism has been reworked. Does this make more sense? I generally don't think that logic that relies only on gemd-python concepts should be implemented in a downstream package, and it seemed like implementing it as a gemd.util method was going to be messy. I still would have liked a cleaner way of accessing the PropertyAndCondition elements.
There was a problem hiding this comment.
Yes, I think that this makes more sense.
|
|
||
| for typ in (HasParameters, HasConditions, HasProperties, HasSpec, HasTemplate): | ||
| if isinstance(self, typ): | ||
| result |= typ.all_dependencies(self) |
There was a problem hiding this comment.
Why use the in-place union operation here, but result.add below? They seem to me like they both do the same thing.
There was a problem hiding this comment.
add adds a single element to the set, where as |= adds all elements of a second set (like append & extend for lists). If you'd prefer all method calls, result |= could be changed to result.update()
There was a problem hiding this comment.
@bfolie For clarity, I did not change |= to result.update() in my last push. Should I have?
3 participants
This is an initial effort to migrate _all_dependencies into gemd-python. At present, recursive_foreach and recursive_flatmap can crawl a structure to recover all objects but cannot do so in a way that reveals layers that are helpful in batching.
The current structure of the method calls don't feel quite right, and at least feel like they motivate creation of a has_spec mixin.