Checks for the Masterdata Checker

[carlosmada22]

20231201_Rules_for_Checking_Masterdata.docx

Here are presented the first rules for checking the BAM Masterdata, developed originally by Rukeia.

This discussion is intended to evaluate and add, remove, or change the different rules to be applied by the Data Store Team.

[JosePizarro3]

I have re-visited the Word document, and layout the steps we need to do for the checker.

Some considerations before the TODOs:

1. The data model is defined in this repo (aka "bam-masterdata" + in individual use-case specific repos controlled by the DSSt (aka "individual DSSt repos")
2. The individual DSSt repos use bam-masterdata as a dependency
3. The individual DSSt repos have a Github action doing the checker action with respect to the bam-masterdata repo
4. bam-masterdata and the individual DSSt repos uses the checker before push-to-openbis is applied
5. In each individual DSSt repo, the maintainers must decide one or another schema writing, excel or Python, not both: this is too much for us to implement right now

So we need a checker CLI #12 + a push-to-openbis CLI #13 applying the checker before pushing. Now, the checker should be applied Python-to-Python, i.e., we need to work with exports from excel to Python. Meaning, the excel needs to be exported to Python using a import-from-excel CLI #45

---

import-from-excel behavior #45

• Create CLI import_from_excel which accepts
  • --excel-file: path to the excel file.
  • optional option --to: path to export to a specific path to save the generated Python modules
• check excel formatting: this is done when reading and using the checker -> try/except with logger.critical("The format of the excel masterdata file does not fit with the standard openBIS format")

---

checker behavior #12

In bam-masterdata

• Create CLICK check which accepts:

  • --file-path: path to the data model to be tested. It can be an Excel file, a Python file, or a directory with Python files.

• For each entity type:

  • Stores all logger inside each entity under a field _log_msgs list of strings
  • If code exists:
    • For property types:
      • If data_type has changed w.r.t. openBIS: logger.critical(f"Property type {code} has changed its `data_type` value from {data_type_in_openbis} to {data_type}, which means that data using a type that is not compatible with the new type will probably break openBIS. You need to define a new property with the new data type or revise your data model.")
      • If data_type == "CONTROLLEDVOCABULARY" and vocabulary_code has changed w.r.t. openBIS: logger.critical(f"Property type {code} using controlled vocabulary has changed its `vocabulary_code` value from {vocabulary_code_in_openbis} to {vocabulary_code}, which means that data using a type that is not compatible with the new type will probably break openBIS. You need to define a new property with the new data type or revise your data model.")
    • For any entity type (including properties):
      • If any other attribute (description, property_label, validation_script, e.g.) has changed wr.t. openBIS: logger.warning(f"Entity type {code} has changed its attribute {key} from {value_in_openbis} to {value}.") JMP: note I rejected most of the checks in the Word as these are not necessary
    • When properties are assigned to entities like ObjectType, CollectionType, DatasetType or terms (aka "properties" from now on) are assigned to VocabularyType:
      • If property code does not exist in openBIS: logger.error(f"The assigned property {code} to the entity {cls_name} does not exist in openBIS. Please, define it in your PropertyType section.") JMP: I am not entirely happy with this, but there is no other way of controlling the assignment of properties, unless we automate this somehow
      • Else:
        • If all assigned properties code coincide with another entity assigned properties code list: logger.info(f"The entity {cls_name} has the same properties defined as {cls_name_of_the_other_entity}. Maybe they are representing the same entity?.") JMP: should we add some sort of "suggestion" message to the logger?
        • If all assigned properties code has changed w.r.t. openBIS: logger.warning(f"The assigned properties to {cls_name} has changed:")
          • If deleted: logger.warning(f"{prop.code} has been deleted.")
          • If added new: logger.info(f"{prop.code} has been added as a new property.")
        • If any other attribute (mandatory, show_in_edit_views, section) has changed: logger.warning(f"Assigned property {prop.code} to entity type {code} has changed its attribute {key} from {value_in_openbis} to {value}.") JMP: note I rejected most of the checks in the Word as these are not necessary
  • Else: pass

• Reads all entity types

In individual DSSt repos

• Create CLI check which accepts:
  • --excel-file (same behavior as in import-from-excel):
  • --python-path:
    • individual Python modules
    • directory with all Python modules to be checked
• Reads all entity types. Reads all bam-masterdata entity types.
• For each entity type:
  • Stores all logger inside each entity under a field _log_msgs list of strings
  • If cls_name exists in bam-masterdata: logger.critical(f"The entity {cls_name} already exists in `bam-masterdata`. Please, check your classes.")
  • Else: pass
• When this is done, apply same check w.r.t. openBIS as in bam-masterdata

---

push-to-openbis behavior #13

• Create CLI push_to_openbis which accepts:
  • --excel-file (same behavior as in import-from-excel):
  • --python-path
    • individual Python modules
    • directory with all Python modules to be checked
• Applies checker
• For each entity, uses _log_msgs:
  • If checker has been successful (no critical, error, warning), creates or updates entity in openBIS
  • Else: logger.info("The checking of the entity type {cls_name} did not pass. Please, check your entity definitions.

[angelaari]

Additional Checks for the Masterdata Checker that I did not find in the list:

1. Property $ANNOTATIONS_STATE is deprecated for openBIS 20.10.7.3.

2. "Section"

• Section names: First letter of every word starts with capitalized lettter
• Section "Additional Information" is followed by Section "Comments"
• Property "Notes" exists in Section "Additional Information"
• Property "Comments" exists in Section "Comments"

[JosePizarro3]

Although this might go beyond the Masterdata checker as it checks the Data and not the Masterdata of the DS-instance.
Would it be possible that the Masterdata checker also checks if an Entity type is already used in openBIS (Object, Collection, Datasets)?
If so, the warning would be useful:
"Property type code" exists in "Entity code"

Yes, we can add this, it makes sense to have an utility to query the DS and see if a object/property are being used. But I'd say we can leave it for a future implementation and focus now on the "static" schema to which the checker is checking for.

[carlosmada22]

• If all assigned properties code coincide with another entity assigned properties code list: logger.info(f"The entity {cls_name} has the same properties defined as {cls_name_of_the_other_entity}. Maybe they are representing the same entity?.") JMP: should we add some sort of "suggestion" message to the logger?

This is not correctly conditioned, since this needs to be checked EVEN if the entity is a new entity to be registered in the system, but with how the conditions are defined right now, this is never checked, because when an entity is not in the system, all the checks are skipped.

Except if this is only applying for entities already present in the system, and not for new ones.

Can this be clarified? @JosePizarro3 @angelaari

[carlosmada22]

• If all assigned properties code coincide with another entity assigned properties code list: logger.info(f"The entity {cls_name} has the same properties defined as {cls_name_of_the_other_entity}. Maybe they are representing the same entity?.") JMP: should we add some sort of "suggestion" message to the logger?
• If warnings will be included:
  Warning: Entity will not be imported in openBIS.
  @JosePizarro3 this warning is based on the Masterdata import process as done until now and restricted to Instance Admins (DS-Teams)

Does this means that, if the set of assigned properties is equal to another existing entity, the only log that should appear is:
"Warning: Entity will not be imported in openBIS." ??
Without giving any more information about the reason why this is happening? Or you mean just adding this part of the log to the rest of the information?

[carlosmada22]

We need to check this part too:

• If property code does not exist in openBIS: logger.error(f"The assigned property {code} to the entity {cls_name} does not exist in openBIS. Please, define it in your PropertyType section.") JMP: I am not entirely happy with this, but there is no other way of controlling the assignment of properties, unless we automate this somehow
  - [x] Else:
  - [x] If all assigned properties code coincide with another entity assigned properties code list: logger.info(f"The entity {cls_name} has the same properties defined as {cls_name_of_the_other_entity}. Maybe they are representing the same entity?.") JMP: should we add some sort of "suggestion" message to the logger?
  - [x] If all assigned properties code has changed w.r.t. openBIS: logger.warning(f"The assigned properties to {cls_name} has changed:")
  - [x] If deleted: logger.warning(f"{prop.code} has been deleted.")
  - [x] If added new: logger.info(f"{prop.code} has been added as a new property.")
  - [x] If any other attribute (mandatory, show_in_edit_views, section) has changed: logger.warning(f"Assigned property {prop.code} to entity type {code} has changed its attribute {key} from {value_in_openbis} to {value}.") JMP: note I rejected most of the checks in the Word as these are not necessary

Here, the first "If" clause, is referring to just one property "code". But the "Else" is referring to ALL the properties, so both are being executed for the moment and all the logs are printed if this situation appears. I.m.o, this needs to be clarified,

[carlosmada22]

@JosePizarro3 Currently, the tool import_from_excel reads the excel file indicated in the --path option in the CLI, and performs the same workflow as the fill_masterdata does, but with a different origin.

Also, during this conversion, the content checks for the Excel file (those gathered in the document above) are performed at the same time. There are two things to define here:

1. Do we want to perform this checks ALWAYS that the Excel is converted? (I think this makes sense, since you will want to check if the cell values are correct or not). The alternative will be specifying in the CLI with an option if we want the checks to be performed or not.
2. Do we want to perform the conversion to Pydantic classes coming from the excel ALSO if there are failing checks (obviously letting the user know that there are errors in the file)? Or do we prefer to just show all the check errors and if there are ANY error, not perform the conversion? There is a 3rd option here: Define which errors are critical, and cancel the process if some of these happens, but continue if there are just "warnings"
3. Do we want to just log the errors by default? Should we add another option to export the errors in a file (like --output), apart from just printing them? Or should the file saving be the main behaviour?

[JosePizarro3]

@JosePizarro3 Currently, the tool import_from_excel reads the excel file indicated in the --path option in the CLI, and performs the same workflow as the fill_masterdata does, but with a different origin.

Also, during this conversion, the content checks for the Excel file (those gathered in the document above) are performed at the same time. There are two things to define here:

1. Do we want to perform this checks ALWAYS that the Excel is converted? (I think this makes sense, since you will want to check if the cell values are correct or not). The alternative will be specifying in the CLI with an option if we want the checks to be performed or not.
2. Do we want to perform the conversion to Pydantic classes coming from the excel ALSO if there are failing checks (obviously letting the user know that there are errors in the file)? Or do we prefer to just show all the check errors and if there are ANY error, not perform the conversion? There is a 3rd option here: Define which errors are critical, and cancel the process if some of these happens, but continue if there are just "warnings"
3. Do we want to just log the errors by default? Should we add another option to export the errors in a file (like --output), apart from just printing them? Or should the file saving be the main behaviour?

I'll try to answer with my point of view:

• 1 and 2: Yes. Otherwise, if the export failed because of some weird naming we will have anyways some unexpected raised errors OR pydantic will complain when validating the fields following our schema. We are simply swapping unexpected raised errors with detailed messages in the logger, and the pydantic we are solving before any issues arise (instead of doing it by hand, simply by doing the script doing this for us on the first place).
• 3: You can always print the output of a CLI adding > logs.txt after the CLI used. It will save the logs to a specific filepath.

[JosePizarro3]

@carlosmada22 I will answer you in separate threads to not get confused.

If all assigned properties code coincide with another entity assigned properties code list: logger.info(f"The entity {cls_name} has the same properties defined as {cls_name_of_the_other_entity}. Maybe they are representing the same entity?.") JMP: should we add some sort of "suggestion" message to the logger?

This is not correctly conditioned, since this needs to be checked EVEN if the entity is a new entity to be registered in the system, but with how the conditions are defined right now, this is never checked, because when an entity is not in the system, all the checks are skipped.

Except if this is only applying for entities already present in the system, and not for new ones.

Can this be clarified? @JosePizarro3 @angelaari

I am not happy with this condition either. In my opinion, we can delete it.

My reasoning goes into the fact that a lot of entities can coincide with the assigned properties of others, and the separation is purely semantic. But this is important, so I won't clutter the logs with this information.

[carlosmada22]

Perfect, right now the check is being performed always, new and existing entities, so if @angelaari agrees, we can just leave it as an "info" log.

[JosePizarro3]

If all assigned properties code coincide with another entity assigned properties code list: logger.info(f"The entity {cls_name} has the same properties defined as {cls_name_of_the_other_entity}. Maybe they are representing the same entity?.") JMP: should we add some sort of "suggestion" message to the logger?
If warnings will be included:
Warning: Entity will not be imported in openBIS.
@JosePizarro3 this warning is based on the Masterdata import process as done until now and restricted to Instance Admins (DS-Teams)

Does this means that, if the set of assigned properties is equal to another existing entity, the only log that should appear is:
"Warning: Entity will not be imported in openBIS." ??
Without giving any more information about the reason why this is happening? Or you mean just adding this part of the log to the rest of the information?

For the same reason as above, I would simply delete this condition: a lot of entities can coincide in the assigned properties with others, and simply differentiate by semantics/descriptions.

[carlosmada22]

Same here, the condition is already coded, so we can leave it just as an "info" log.

[JosePizarro3]

We need to check this part too:

If property code does not exist in openBIS: logger.error(f"The assigned property {code} to the entity {cls_name} does not exist in openBIS. Please, define it in your PropertyType section.") JMP: I am not entirely happy with this, but there is no other way of controlling the assignment of properties, unless we automate this somehow

• Else:
• If all assigned properties code coincide with another entity assigned properties code list: logger.info(f"The entity {cls_name} has the same properties defined as {cls_name_of_the_other_entity}. Maybe they are representing the same entity?.") JMP: should we add some sort of "suggestion" message to the logger?
• If all assigned properties code has changed w.r.t. openBIS: logger.warning(f"The assigned properties to {cls_name} has changed:")
• If deleted: logger.warning(f"{prop.code} has been deleted.")
• If added new: logger.info(f"{prop.code} has been added as a new property.")
• If any other attribute (mandatory, show_in_edit_views, section) has changed: logger.warning(f"Assigned property {prop.code} to entity type {code} has changed its attribute {key} from {value_in_openbis} to {value}.") JMP: note I rejected most of the checks in the Word as these are not necessary

Here, the first "If" clause, is referring to just one property "code". But the "Else" is referring to ALL the properties, so both are being executed for the moment and all the logs are printed if this situation appears. I.m.o, this needs to be clarified,

I think this condition was because we have property_types.py. I am not happy with it either, as it implies that users need to define a priory the properties that they assigned. IMO, this should be automated, i.e., our bam-masterdata export tools make sure that all assigned properties are defined and the definitions are consistent.

We can discuss this in more detail once I am back.

[carlosmada22]

Nice, then I will leave it as it is now (performing all the checks, to every property that is not assigned, and then the rest of the checks mentioned here), and we can modify it later if needed. Thank you Chema!

[JosePizarro3]

Happy to say that this is done since you merged the checker, @carlosmada22. Do you want to close the discussion or leave it open?

[carlosmada22]

Nice! I will leave it open for the moment, just in case that during the Rollout phase we see the need of adding/removing/modifying some of the current checks applied.

[angelaari]

@carlosmada22 Is this really an Error "Auto generated codes not found in the headers.".

This error is displayed when checking the Object Type: ORGANISM as downloaded from the main Data Store instance.

[JosePizarro3]

This is solved: the issue was in using the past tense in "generated". New bam-masterdata version 0.5.0 has a fix for this.

For the checker app, we would need @jraedler to update the deployment of the app locally

[angelaari]

@carlosmada22 the checker does not perform any action with the attached controlled vocabulary. Do you know why?
vocabulary_ORGANISM_RISK_GROUP_v1_1.8_konthur.xlsx

[angelaari]

This is not working, based on the Wiki article and for consistent naming with other Entity Types. Please fix this in the checker and allow to check controlled vocabulary which file name starts with e.g., "vocabulary". However, before deciding this,
@JosePizarro3 lets please agree the naming convention for Entity Types.
So far the naming convention for Excel files as defined in Wiki were:

• the entity type: collection_type, object_type, dataset_type, or vocabulary
• For the first version of the Object type INSTRUMENT defined by the user "mmuster" from VP.1, the Masterdata Excel file should be named object_type_INSTRUMENT_v1_VP.1_mmuster.xlsx.

Based on this, the CVs the files should start with "vocabulary"...

[JosePizarro3]

there might be something else going wrong; the checker does not check file names or tab names in the excel, I think, but let me double check

[angelaari]

the CV that I was using for the test
vocabulary_dfg_device_code.xlsx

[angelaari]

can you please share here an excel file of a controlled vocabulary that works for the checker.
As Chema says the Masterdata checker works but it will take that JR update the App for the Masterdata Checker

[JosePizarro3]

The allowed names for the sheets are: Object types and Vocabulary types

#160

I added this issue to make the checker more robust, so the format of the sheets in the excel, or file name, or if it is one or multiple excels do not matter

[angelaari]

@carlosmada22
Two corrections concerning check for Property label COMMENTS

1. The checker reports that COMMENTS is mising, although this Property label exists
   Current Error message "Missing required property 'COMMENTS' in section 'Comments'."
   Two Excel files were tested
   object-type_ORGANISM.2025-07-14.xlsx

object_type_CHEMICAL_v1_S.3_cmadaria.xlsx

2. Error message needs use headers´s names (Code, Property label, etc.)
   Suggested error message could be:
   Missing required Code ($XMLCOMMENTS or Property label (Comments)

[JosePizarro3]

#154

[JosePizarro3]

Ok, fixed! Now the checker looks correctly for $XMLCOMMENTS to be under the section 'Comments'

[caro-demi]

#154
it would be great, if this messages also showed the cell (for example: Cell $X$Y: Missing required property '['comments', 'xmlcomments', '$xmlcomments']' in section 'Comments'). In case one checks multiples object types in one excel file, it's hard to identify where exactly the error occurs.

[angelaari]

**Tested Object Type: **object-types.xlsx
Error: Invalid value 'EXPERIMENTAL_STEP.SEM.misc_validator.py' at row 3, column 4 in sheet Sheet0

The error message is missliding. openBIS checks automatically if the validation script exists within a folder together with the Object Type to allow import (by Admins). The users reading this error message won´t have clear what to do.

The checker can give rather a warming message for ADMINs but it does not seem necessary, as said openBIS gives automatically a warming during import of Object Types
The name of the validation script should match the name of the imported file.

[JosePizarro3]

I guess validators, overall, could be deprecated. I don't see why to add such scripts attached to objects, but rather the script logics should be part of the class methods itself (via a process that is typically called normalization)

[angelaari]

Version in Excel file for Entity Types import required?
@carlosmada22 Are you aware if version number is still required for importing Entity types in the current openBIS version (20.20.22.1) the Data Store uses?
I noticed the checker is not giving error to missing version number. Is version deprecated for openBIS 20.20.22.1?

The Masterdata checker seems to perform inconsitently for checking version. the checker displays error in of two files checked, although none of them include version.
Tested files:
object_type_CHEMICAL_v1_S.3_cmadaria.xlsx
object-type_ORGANISM.2025-07-14.xlsx

I ran a test import for an Object Type in training instance but failed to clear errors. Could you please run a test to clarify if the version is still required for import and therefore, the checker should give an error or not.

The files that I was using for a test are:
Object_type_Test1.xlsx
object-type_TEST_1.xlsx

The masterdata-template from the openBIS documentation. https://openbis.readthedocs.io/en/20.10.x/user-documentation/general-admin-users/admins-documentation/new-entity-type-registration.html#register-masterdata-via-excel

[JosePizarro3]

I ran a test import for an Object Type in training instance but failed to clear errors. Could you please run a test to clarify if the version is still required for import and therefore, the checker should give an error or not.

This is not relevant anymore: we won't be using the excel import for the openBIS instances to define new objects, but rather the CLI in this repo.

[angelaari]

Thanks for fixing previous bugs!
Can you please check the validity of the following errors? It seems to me that something can be fixed for RESPONSIBLE_PERSON and CO_RESPONSIBLE_PERSON

1. Test with Chemical.xlsx displays following Errors:

ERROR: Invalid data type value found in the Data type column at position F22 in Tabelle1.The Data Type should be one of the following: ['BOOLEAN', 'CONTROLLEDVOCABULARY', 'DATE', 'HYPERLINK', 'INTEGER', 'MULTILINE_VARCHAR', 'OBJECT', 'SAMPLE', 'REAL', 'TIMESTAMP', 'VARCHAR', 'XML']

ERROR: Invalid 'SAMPLE:PERSON.BAM' value found in the 'data_type' field at line ('A22',) in entity 'CHEMICAL' of 'tabelle1' in 'properties'. The Data Type should be one of the following: ['BOOLEAN', 'CONTROLLEDVOCABULARY', 'DATE', 'HYPERLINK', 'INTEGER', 'MULTILINE_VARCHAR', 'OBJECT', 'SAMPLE', 'REAL', 'TIMESTAMP', 'VARCHAR', 'XML']

2. Test with SAMPLE.xlsx
   displays following errors:

[JosePizarro3]

Last thing: @angelaari is the chemical excel you shared directly downloaded from our openbis instances 20.11?

[angelaari]

@JosePizarro3 thanks and yes the Chemical excel is exported from the main Data Store instance openBIS version 20.10.11.1

[carlosmada22]

I can implement it, but I did not fully understand what you mean, @JosePizarro3 could you please tell me what exactly I need to modify in the checker to make this work? If I am correct, you meant that the checker should accept this object type but modifying the cell content?

[JosePizarro3]

So, we could:

1. Change the data_type from OBJECT to OBJECT:<object_code>, or
2. compose it in the checker directly by combining (or breaking, if the checker reads the excel)

I would prefer option two. Something like:

...
if dataType.startswith("OBJECT"):
    els = dataType.split(":")
    if len(els) != 2:
        # error msg here
    data_type_from_excel = "OBJECT"
    object_code_from_excel = els[-1]
    # ...checks here

[angelaari]

@carlosmada22 Hi, Just a quick reminder after the holidays! Today's test with the previous file ‘SAMPLE.xlsx’ still shows the same errors.