Add SOP0012 for Withdraw Dataset from Node Allele Frequency Beacon - #73
Add SOP0012 for Withdraw Dataset from Node Allele Frequency Beacon#73costero-e wants to merge 30 commits into
Conversation
|
I have added @AlesCUMSI to the review as he is an active beacon user and I think he can provide some help. Thank you, @AlesCUMSI. Let me know if you don't have the time now to do it and I will choose someone else. |
AlesCUMSI
left a comment
There was a problem hiding this comment.
Looks good overall 👍 I’ve left a few minor suggestions regarding wording, clarity, and consistency.
| ```bash | ||
| docker exec mongoprod /bin/bash -c 'mongosh beacon -u <user> -p <password> --authenticationDatabase admin --eval "db.datasets.deleteMany({\"id\": \"<id>\"})"' | ||
| ``` | ||
|
|
There was a problem hiding this comment.
You have redinex in both hard deletions, but not here. Is this correct?
There was a problem hiding this comment.
Yes, because in the soft one, the variants are never removed 👍
There was a problem hiding this comment.
If variants are never removed, then can the effects of the presence of that data propagate to other datasets?
In other words, the effective deletion of the dataset would not be real, as it would not change the AFs?
There was a problem hiding this comment.
The AFs are statically calculated, not on the fly. There's confusion about this, because we are not allowing progressive updates. Everything works as a single package. This is what was agreed in GDI, as for now. So, no, no propagation possible.
|
I have now finished adding @AlesCUMSI suggestions. Thanks for the useful feedback @AlesCUMSI , it helped a lot! |
M-casado
left a comment
There was a problem hiding this comment.
Great work @costero-e . Also thanks to @AlesCUMSI , as many of my comments went in line with his.
I have some comments that I left in line, and some general comments here:
- If the soft deletion is defined as data being retained, isn't it inconsistent to use
db.datasets.deleteManyto delete the dataset from mongoDB? I'm not familiar with AF Beacon, so maybe it's not, but the command sounds like we're deleting data. - The
dataset_config.ymlreferenced as per the documentation (inmain) here contradicts theisDeprecatedin this SOP. EitherisTestandisSyntheticare not the only values, or... something is off - Verification in step 4 doesn't distinguish deletion type: it says to verify "datasets and variants" queries for both types, but if I went through the soft-deletion path, I only removed dataset records (not variants).
- Not sure if this is meant in the Beacon spec, but
isDeprecatedflag applies to both types of deletion without explanation. For hard-deletion everything is already deleted, so would we be adding a flag to a dataset that is gone? Is this the way to audit changes in the Beacon?
| | API | Application Programming Interface | | ||
| | BEN | Beneficiary | | ||
| | CC | Carbon Copy (used in communications to send a copy to others) | | ||
| | CRG | Centre for Genomic Regulation | |
There was a problem hiding this comment.
Missing "AF" for Array Frequency
There was a problem hiding this comment.
In both the charter and the SOP0012
There was a problem hiding this comment.
If i might make a suggestion based on @M-casado comment (If the soft deletion is defined as data being retained, isn't it inconsistent to use db.datasets.deleteMany to delete the dataset from mongoDB? I'm not familiar with AF Beacon, so maybe it's not, but the command sounds like we're deleting data.): What if we nothing is deleted soft-delete, mainteiner just needs to remove dataset id from beacon/permissions/datasets/datasets_permissions.yml file? If my memory serves me correct, this prevents beacon from returning dataset (and g-variants connected to it)?
There was a problem hiding this comment.
Regarding the soft-deletion, if we only modify the datasets_permissions.yml file, the dataset will continue appearing in the /datasets endpoint, which for beacon means: we have this dataset and you will be able to retrieve it depending on your permissions. I think this is not the situation we want, exactly. Deleting just the datasets entry will block any beacon response to return anything from this dataset. This is a very small record, that only contains metadata for the dataset, and I think is just what a soft-deletion pursues, deletion of metadata regarding a dataset (at the end is soft-deletion, something, very minimal, has to be deleted). What hard-deletion does is deleting the much bigger amount of variants, which can't be rolled back with a simple insert. For me, the approach I did is correct.
|
|
||
| | Step identifier | When | Who | | ||
| | :-------------- | :--------------------------------------- | :-------------------------------------- | | ||
| | `4` | After successful completion of Step 3 | Node aggregated beacon maintainer | |
There was a problem hiding this comment.
The issue I have with the current steps, is that I think we're missing one that is for failure. Otherwise, steps 2 redirect you to step 3 on failure of withdrawal, and then you come here and it's phrased as if the withdrawal was successful, and to verify the records are not appearing. Hence why I would have a step of failure in the diagram/procedure, to be explicit with the logic
…le-frequency-beacon.md Co-authored-by: Marcos Casado <mcasado@ebi.ac.uk>
…le-frequency-beacon.md Update on actor of the step 1 Co-authored-by: Marcos Casado <mcasado@ebi.ac.uk>
…le-frequency-beacon.md VHD report clarification Co-authored-by: Marcos Casado <mcasado@ebi.ac.uk>
…le-frequency-beacon.md Co-authored-by: Marcos Casado <mcasado@ebi.ac.uk>
| | Author | Liina Nagirnaja | Beacon Manager | CRG | | ||
| | Author | Jordi Rambla | Beacon Product Owner | CRG | | ||
| | Author | Oriol López-Doriga Sagalés | Beacon Developer | CRG | | ||
| | Author | Aleš Čep | Task 4.3 member | CRG | |
There was a problem hiding this comment.
I am not sure why my name is added as Author. This must be mistake. If any, maybe change it to Reviewer. Also, my organization is not CRG, it is a University Of Maribor (UM)
|
@costero-e - I was wondering if after you addressed @AlesCUMSI's and my feedback on this PR, it is ready for the approval round of the OC and SDPC in GDI. I've seen a few comments still pending, but some are now outdated. Let me know your thoughts so that we can continue where we left |
|
Hi @M-casado, I re-reviewed the SOP, executing all the commands and I think that the comments pending mostly are outdated and the only thing I see a bit hanging is the FAIR principles one (find datasets that were removed previously from the beacon) that is dependant on a greater solution of the specification. I could do a workaround but would be only implementation-specific. If we can move on without this, then, the SOP is ready. |
|
Great @costero-e - As an author of other SOPs myself, I'm aware that there's always room for improvements, but I agree it's best we get the GDI OC/SDPC involved already, as the SOP framework is built around continuous revision. Especially with the feature requiring a spec change. Great work, once again! |
|
Just communicated the request to the OC/SDPC |
Perfect then. And thanks to you for your invaluable contributions! |
|
Hi Oriol, Thanks for the corrections. In the section about "Remove dataset permission", I think it would be useful to remind the operator to save the relevant information for auditing purposes before removing the dataset from datasets_permissions.yml, rather than only describing the removal step, specially for controlled access. Also, in the diagram, nodes K and L appear to have the same label, which results in "Remove dataset permissions" being shown twice. |
|
Hi @omllobet, Thanks a lot for your useful feedback. I have addressed now the identified issues. Let me know if you find anything else that is not making sense. Best! |
|
I approve this SOP as a member of the OC |
|
I approve this SOP as a member of the SDPC |
|
Status summary:
|
|
@costero-e - Please add the approvers with their roles to the table in the SOP before merging |
|
Done @M-casado |
…le-frequency-beacon.md Co-authored-by: Marcos Casado <mcasado@ebi.ac.uk>
Summary
GDI-SOP0012_withdraw_dataset_from_node_allele_frequency_beacon.md.Types of changes
Motivation and Context
Issue #65 requested a node-specific SOP describing how a node allele frequency beacon should handle dataset withdrawal. This PR adds the allele frequency beacon of that workflow as a reusable node-specific template.
References
Changes Introduced
Review
The SOP content is based on the reviewed draft prepared for issue #65 and converted into the repository template. In this PR, the SOP is limited to deletion of the dataset from the allele frequency beacon. Full or partial withdrawal don't apply as frequencies are calculated per dataset and statically, meaning that the removal of a dataset doesn't affect other datasets and global counts are to be made on the client side and on the fly.
Additional Notes
Checklist:
General Compliance:
Only applicable if the PR includes new, or changes to, GDI SOPs (i.e., documents at
sops/):