Repository navigation
fix: de-duplicate identical pipelines when combining Processing objects - #1875
seanmcculloch wants to merge 1 commit into
Conversation
7257d8b to
e2afa21
Compare
| seen = set() | ||
| output_list = [x for x in lst if not (x in seen or seen.add(x))] | ||
| except TypeError: | ||
| # Unhashable elements (e.g. pydantic models): fall back to equality |
There was a problem hiding this comment.
Eg, removing duplicate aind_data_schema.components.identifiers.Code objects like pipelines
|
How can this happen -- also isn't this a risk if someone runs the same code in multiple branches of a pipeline? |
|
@dbirman We could end up with duplicate entries in the de-duping used to be handled my the metadata manager: https://github.com/AllenNeuralDynamics/aind-metadata-manager/blob/3d342db6a8aea4a5e157d0686de0ed913dd7a3ce/src/aind_metadata_manager/metadata_manager.py#L416-L440 I am currently working on this PR, which drops a lot of aggregation logic in the metadata manager, in favor or relying on the data schema itself (through BUT: it is risky in a way - since the description notes that aind-data-schema/src/aind_data_schema/core/processing.py Lines 105 to 112 in f38cb0f However, |
|
Individual capsules in a pipeline shouldn't be writing the pipeline though? How would they "know" that they are inside of a pipeline? It's the manager during aggregation that should assign them all the pipeline_name and add that object. |
dbirman
left a comment
There was a problem hiding this comment.
I'll go a bit farther than my comment above -- I don't think this is logic that the schema should be implementing. I think this kind of management has to happen downstream of the merge code, which we should try to keep as minimal as possible.
|
@dbirman The intention is that this will be called by the metadata-manager, as it is right now. just that this PR is moving that code from the metadata-manager into the data-schema itself. Perhaps it should stay in the metadata-manager for now then? The change here itself only removes exact duplicates within the Can you confirm that this PR should be closed and that logic should stay in the metadata-manager? |
|
I think the situation that this PR is solving for, which is roughly: "Two identical pipelines were run in parallel and are now being merged" is a situation that should never happen, which is why I'm pushing back. I think that's actually not what you're trying to fix though, you're trying to fix: "A single pipeline has capsules whose processing needs to be merged, but each of the capsules wrote their own pipeline information". That's a bug in the capsules -- capsules (in my opinion) should not write pipeline information at all. It's only at the last step in the pipeline when all capsules are done that the metadata manager should capture all the individual capsule processing and then wrap them all with the pipeline info. Does that make more sense? Hop on a call if it's not clear. |
Currently, the metadata manager merges Processing from multiple upstream sources, that usually share a pipeline, and it used to de-dup those itself. Moving that aggregation onto the schema's "+" operator and
from_metadatalost the de-dup, since Processing.add concatenates pipelines without removing duplicates. This restores it in add (keyed on fullidentifiers.Codeidentity/__eq__, not name), so every + consumer gets it and the manager can drop its local shim.What changed
Processing.__add__concatenatedpipelineswithout de-duplication, so combining N Processing objects that share a pipeline produced N identicalCodeentries. It now removes duplicates after merging.remove_duplicatesgains an equality-based fallback so it handles unhashable elements such as pydanticCodemodels, not just hashable scalars.Review notes
nameare kept — identity is full model equality, not name.