Add SOP0014 Add datasets metadata to node beacon - #75
Conversation
costero-e
changed the title
|
Hi @M-casado, the PR is now ready for OC review. Thanks. |
|
Hi @costero-e , why was the reference from SOP0009 removed at 546d7df? |
The PR was giving error due to the file not existing. I thought the file was missing, but now I see the problem was the relative path, which was wrong. It's a bit odd that we inherit the mistakes from other procedures and we have to address them in new PRs every single time. Also, I don't know how is this going to be managed, but conflicts will arise trying to merge same file from different PRs. I will address it anyways. |
|
The workflow results are useful but need to be interpreted. Similar to how we kept the Regarding the errors from other SOPs being inspected, the idea was to assess the whole branch state before merge. Initially we didn't expect SOPs to have "justifiable" errors (such as the unresolvable references above) trailing for a while until an official release is made. We could review the workflows to trigger only on edited SOPs, although that would still be flagging the error in this particular case. We can check the merge issues when they arise. I would assume it's rather uncommon, at least with the current setup. |
|
Wait but the SOP I removed is the withdrawal AF Beacon dataset, which is the SOP0012, because I copy pasted the file from SOP0012. Do we have to reference this SOP0014 in the SOP0009? |
|
@costero-e - You are 100% right - It's not the I see what you mean about the merging conflicts, since you removed a part that you added in the source branch. In this case I would say our options are:
|
|
Yes, perfect, I also got confused (a lot of things going on). The conflicts will happen probably (if they don't happen better for us), but they are manageable, I'm not saying there's a better way to do the things but IMHO it doesn't make sense to try avoiding them by reproducing the other PRs here. Let me know if I can do anything else or I will wait for the official review. Thanks for your always useful help @M-casado! |
|
No action needed for now, @costero-e, just set a reminder that if there are no conflicts on either this PR or #73, we need to make sure that the reference from This PR can go into the approval stage now, since the G. Doc was reviewed already by @waakanni and @AlesCUMSI -- Thanks a lot for your reviews as well!
Maybe I made a mistake with the linting suite for CI/CD? As far as I can remember the workflows do not take the content of the other PRs/branches. They take the "current state of this branch (including all SOPs in this current branch, as a block) and test them against the suite of tests". As far as I know the workflow errors in this PR don't appear because there are other PRs with new SOPs open, but because this "branch" as a block was either:
If this is not what you experienced please let me know because that means I need to amend the workflows/linting tests. |
|
Review 1 from the OC: Good SOP, just section 8.4 references 'deleting' a dataset instead of inserting, and the section on updating the dataset and does not mention what to do with an updated dataset which is defined in section 8.3. General comment - I like that the detail is in the SOP on how to technically perform this action, but it would also be good to automate this using a script and a set order, e.g. update the FDP and then use a script to pull or check the ID from the FDP and then perform the desired action on the Beacon - this would ensure the data in both are identical. However this could be a node specific task who could share the script once created. The same comment goes for SOP00014. |
|
Hi @jdylan. Thank you for the feedback. I have addressed section 8.4.
Although I believe it is wise to do as you say, as this is clearly a beacon specific SOP I think this type of procedure affecting more than one component should be part of a broader protocol, IMHO. |
|
|
||
| ### 4. Purpose | ||
|
|
||
| Data loaded into GDI, be it through beacon or through FAIR Data Points (FDP), need to be consistent and queryable. In order to do so, validation of the data prior to submission needs to be performed. |
There was a problem hiding this comment.
need or needs?
Also, this sentence looks more like context than to purpose to me.
There was a problem hiding this comment.
Right. I have reworded the paragraph for this:
Data loaded into GDI, be it through beacon or through FAIR Data Points (FDP), need to be consistent and queryable. This SOP aims to clarify how to proceed when new dataset metadata needs to be added to a Node's beacon, covering the cases for updating or inserting datasets and full validation of the incoming metadata.
Let me know if this enlightens better the purpose of the SOP.
|
|
||
| Out of scope for this SOP are: | ||
| - Full GDI HDM entry or validation | ||
| - Uploading variant records to beacon. For MAP1, the specific requirements are covered in the beacon guidelines documentation (here and here). |
| id | ||
| name | ||
| ``` | ||
| Fill in the metadata in the row after the header. Ensure that the id follows the required [link](https://raw.githubusercontent.com/GenomicDataInfrastructure/gdi-metadata/refs/heads/main/Formulasation(shacl)/core/PiecesShape/Dataset.ttl) FDP identifier format and check that it is spelled exactly as the FDP dataset identifier for the same dataset. |
There was a problem hiding this comment.
I would use another name rather than link, i.e Dataset SHACL
Also, it should mention that it needs to be consistent with the SHACL that the Node is currently using in their FDP which doesn't need to be the latest version from the gdi metadata repository
There was a problem hiding this comment.
I have reworded the whole paragraph for:
Fill in the metadata in the row after the header. Ensure that the id follows the required [Dataset SHACL](https://raw.githubusercontent.com/GenomicDataInfrastructure/gdi-metadata/refs/heads/main/Formulasation(shacl)/core/PiecesShape/Dataset.ttl), which needs to be consistent with the SHACL that the Node is currently using in their FDP and doesn't need to be the latest version from the gdi metadata repository.
There was a problem hiding this comment.
As this is a node SOP maybe the language need a little bit of tweaking?
"that the Node is currently using in their FDP" sound a little bit weird to me.
There was a problem hiding this comment.
Ok, I have rephrased for:
It should be consistent with the SHACL version currently used by the Node's FDP, which does not necessarily have to be the latest version available in the GDI Metadata Repository..
| public: | ||
| default_entry_types_granularity: record | ||
| ``` | ||
| This is only an example, set the security level as it is meant for the dataset and add further restrictions as needed. More information about security levels and granularity types can be found here: [link](https://github.com/EGA-archive/beacon2-pi-api/tree/main#making-a-dataset-publicregisteredcontrolled). |
There was a problem hiding this comment.
insted of can be found on link I'd say "can be found in the beacon2 pi repository" or something similar, si it's more accessible
| | :-------------- | :--------------------------------------- | :-------------------------------------- | | ||
| | `6` | After successfully updating new dataset metadata ⏩[Step 3.2](#832-update-metadata-for-existing-dataset), after adding permissions for the new dataset ⏩[Step 4](#84-add-permissions-for-your-new-dataset) or after making additional declarations for your dataset ⏩[Step 5](#85-make-additional-statements-for-your-dataset). | Node beacon maintainer | | ||
|
|
||
| As the node beacon maintainer, proceed to validate your new dataset metadata by running the [beacon verifier](https://beacon-verifier-demo.ega-archive.org/) on your beacon instance. |
There was a problem hiding this comment.
I think this is public software, should we link to the software instead?
There was a problem hiding this comment.
Yes, you are right that we need a backup in case the service is not working. I will add the possibility to download the software locally in case the official service is not up and running.
|
Hi @omllobet. Thanks for your useful review. I have now addressed the different questions you were arising. Let me know what you think about them now! |
|
|
||
| | Step identifier | When | Who | | ||
| | :-------------- | :--------------------------------------- | :-------------------------------------- | | ||
| | `5` | After successfully updating new dataset metadata ⏩[Step 3.2](#832-update-metadata-for-existing-dataset), after adding permissions for the new dataset ⏩[Step 4](#84-add-permissions-for-your-new-dataset) or after making additional declarations for your dataset ⏩[Step 5](#85-make-additional-statements-for-your-dataset). | Node beacon maintainer | |
There was a problem hiding this comment.
this part of the sentence may be extra as we are on Step 5
'or after making additional declarations for your dataset ⏩Step 5'
?
There was a problem hiding this comment.
True! I had mixed the when part for Steps 8.5 and 8.6. Now it's addressed for both if you want to double check. Thanks @omllobet !
|
I approve this SOP as a member of the OC |
|
@jdylan, you commented on this SOP in June & Oriol responded. Are you happy with the current version and the response? Then please approve the SOP on behalf of the OC. Oscar has already confirmed his approval on behalf of the OC. |
|
|
||
| ### 5. Scope | ||
|
|
||
| The SOP covers node-level guidance for uploading dataset metadata to beacon and includes: |
There was a problem hiding this comment.
extra space : The SOP covers node-level guidance for uploading dataset metadata to beacon and includes:
| | :-------------- | :------------------------------------------------------------------ | :-------------------------------------- | | ||
| | `1` | When metadata for a new beacon dataset needs to be added or existing metadata needs to be updated. | Node beacon maintainer | | ||
|
|
||
| After collecting all the information related to the dataset’s metadata, create a new `datasets.csv` file with this exact name inside `/ri-tools/csv` folder (or any subfolder). Copy the headers you need from the template file `/ri-tools/csv/templates/datasets.csv`. Make sure the following mandatory headers are included: |
There was a problem hiding this comment.
I believe the dataset.csv needs to be in a new folder specific for the new dataset or updated dataset. At least this is how I have been doing it. If this is right, then I think it is better to state here that a new folder needs to be created
| docker exec -it ri-tools python csv_to_bff.py | ||
| ``` | ||
| - If the file has been successfully created, proceed to ⏩[Step 2](#82-confirm-the-type-of-operation-to-perform). | ||
| - If you encounter any issues, record the response obtained from the used commands, adding all the information about the actions performed and the intended goal of performing them and report to the GDI Virtual Helpdesk so that requester communication continues through the VHD workflow. |
There was a problem hiding this comment.
should a link to the Virtual helpdesk be added here?
| ```bash | ||
| curl 'https://<yourBeaconDomain>/api/datasets/<id>' | ||
| ``` | ||
| - If the dataset appears and you wish it to declare either if it is test mode, synthetic or deprecated, proceed to ⏩[Step 5](#85-make-additional-statements-for-your-dataset). |
There was a problem hiding this comment.
If the dataset appears and you wish to declare either if it is test mode,
| curl 'https://<yourBeaconDomain>/api/datasets/<id>' | ||
| ``` | ||
| - If the dataset appears and you wish it to declare either if it is test mode, synthetic or deprecated, proceed to ⏩[Step 5](#85-make-additional-statements-for-your-dataset). | ||
| - If the dataset appears, and you don’t wish to make additional declarations for your dataset, proceed to ⏩[Step 6](#86-validate-your-new-datasets-metadata-record-with-verifier-log-and-report-completion). |
There was a problem hiding this comment.
If the dataset appears, but you don’t wish...
| | :-------------- | :--------------------------------------- | :-------------------------------------- | | ||
| | `5` | After successfully updating new dataset metadata ⏩[Step 3.2](#832-update-metadata-for-existing-dataset) or after adding permissions for the new dataset ⏩[Step 4](#84-add-permissions-for-your-new-dataset) | Node beacon maintainer | | ||
|
|
||
| In case you want your dataset to be declared as meant for test mode, specify its nature or deprecate it, you can by editing the `/beacon/conf/datasets/datasets_conf.yml` file. Add a new entry with the dataset id as the main property and add whatever three optional following items you want to declare for the dataset, setting them as `True`. |
There was a problem hiding this comment.
and add whatever three optional following items you want to declare for the dataset, setting them as True. this sentence is missing something or just reads weird
waakanni
left a comment
waakanni
left a comment
There was a problem hiding this comment.
I have suggested some small changes but overall I am happy to approve once they have been addressed.
jdylan
left a comment
jdylan
left a comment
There was a problem hiding this comment.
Subject to the other changes mentioned above
6 participants
Summary
GDI-SOP0014_add_datasets_metadata_node_to_beacon.md.Types of changes
Motivation and Context
Issue #66 requested a node-specific SOP describing how a node beacon should handle adding a dataset to the node beacon. This PR adds the node beacon of that workflow as a reusable node-specific template.
References
Addresses #66Changes Introduced
Review
The SOP content is based on the reviewed draft prepared for issue #66 and converted into the repository template. The SOP covers node-level guidance for uploading dataset metadata to beacon and includes:
Out of scope for this SOP are:
Additional Notes
Checklist:
General Compliance:
Only applicable if the PR includes new, or changes to, GDI SOPs (i.e., documents at
sops/):