suggestion (testing): Consider adding edge case tests for invalid data

Currently, the tests cover complete data and missing fields, but it would be beneficial to add tests for invalid data formats or types to ensure the converter handles these gracefully.

suggestion (testing): Add assertions for specific error messages

It would be helpful to add assertions that check for specific error messages or exceptions when invalid data is passed to the converter. This ensures that the error handling is working as expected.

suggestion (testing): Test for partially missing fields

Consider adding tests where only some fields are missing, rather than all or none. This will help ensure that the converter can handle partial data loss gracefully.

suggestion (testing): Include tests for edge cases with nested structures

The current tests cover basic and missing fields, but it would be beneficial to include tests for edge cases involving nested structures or arrays with unexpected lengths or types.

suggestion (testing): Add tests for invalid nested structures

Consider adding tests that include invalid nested structures or unexpected data types within the nested fields to ensure robust error handling.

suggestion (testing): Test for partially missing fields

Consider adding tests where only some fields are missing, rather than all or none. This will help ensure that the converter can handle partial data loss gracefully.

I think we could easily make this more useful by picking values from a real toy TES run, i.e., put a container image that is actually available, put in a command that actually can be executed in that container, put input files that are permanently available on the web via HTTP(S) (e.g., the raw README.md and LICENSE files from this repo). Please open another issue for that.

It's a bit more tricky for the output, we can think about that in that issue.

Same as above. Also, make sure that all required params are provided (also in the tes example above). For example, the command param in executors is required (see https://github.com/ga4gh/task-execution-schemas/blob/develop/openapi/task_execution_service.openapi.yaml).

Also, I would name these files differently:

• tes_minimal
• tes_full (but then you'd also want to add resources etc.

It's probably also a good idea to have a minimal (required only) and full example (all params available).

Rename files according to suggested example names, e.g.:

• wrroc_from_tes_minimal.json
• wrroc_from_tes_full.json
• wrroc_from_wes_minimal.json
• wrroc_from_wes_full.json

you should use models for data validation, its much simpler and considerably more convenient.

same here, use models for data validation

you can group tests together using comments, that will help improve readability and maintainability further down the line.

Also the way you're currently writing data validation tests will make it difficult to test for complex validation issues, implementing a pydantic model will reduce the need for simple validation tests like this, that way you can focus on possible, edge cases and you can create more complex instances of invalid data and test them at the same time.

+1

Small note: When using pytest, you can also group tests by using classes: https://docs.pytest.org/en/7.1.x/getting-started.html#group-multiple-tests-in-a-class

you can group tests together using comments, that will help improve readability and maintainability further down the line.

Also the way you're currently writing data validation tests will make it difficult to test for complex validation issues, implementing a pydantic model will reduce the need for simple validation tests like this, that way you can focus on possible, edge cases and you can create more complex instances of invalid data and test them at the same time.

Using the model config option, extra = forbid would simplify this and improve readability. Also you need to consider whether you want to throw an error for unexpected fields or if it might provide a better UX to just ignore them.

Also, this is not a complete list of the fields for the WES Data, an error is thrown when you try to run the CLI with wes_full.json as the input.

Thank you for the feedback. I've added extra = "forbid" to the models for better readability and error handling. I'll ensure all required fields for WES Data are included to prevent errors when running the CLI with wes_full.json.

these validators are redundant and cause unnecessary overhead, pydantic will perform automatic validation when you set a type for the fields, which you have already done above.

The comment for the WES Data Model applies here as well

Wouldn't it be expressive enough to call the arg just data in all cases, instead of tes_data, wes_data and wrroc_data? The input "type" is already included in the function names.

Also, please add the following in your next PR for ALL methods/functions (should have already been there, I didn't pay attention during review):

• Type hints for args and return types (as well as for any global and local vars where the type isn't obvious, i.e., that are not assigned the result of a typed function/method); type hints are not required for test cases.
• Google-style docstrings - please also add for classes, modules and packages; please also add for test cases.

See our contributor guide for details.

Please do not add it in this PR, otherwise it will take forever to review and merge it. But make sure it is the very next PR, as it is very important to us to have well documented and typed code.

Thank you for the feedback. I will rename the arguments to data in all cases for consistency. I will also ensure that all methods/functions include type hints and Google-style docstrings in the next PR as per the contributor guide. I understand the importance of well-documented and typed code.

In such cases, it is good practice to chain exceptions, i.e., instead of

except Exception as exc:
  raise Exception(f"My own text: {exc}")

do

except Exception as exc:
  raise Exception("My own text") from exc

Please address this in a future PR for all these cases (but not in this PR).

I will address exception chaining in all relevant cases in a future PR to follow best practices as you suggested.

Okay. Please create an issue for that and then close this conversation.

I think you can remove this comment, the code and variable name are obvious enough.

I will remove the redundant comment in the next PR to improve code readability.

Please use more consistent variable naming: Here, you use wrroc_data, wrroc_filtered_data (wrroc_ first) and validated_wrroc_data (wrroc_ second).

As mentioned above, I don't think wrroc_ is necessary, as it is obvious from the function name. So instead choose either of the following naming schemes consistently:

• data, data_filtered, data_validated (preferred)
• data, filtered_data, validated_data

The reason I personally tend to prefer the first naming scheme is that in a situation where you would enumerate variables (for whatever reason), related variables would be close to each other when alphabetically sorted. It's usually not an issue, but sometimes it is, and so it's useful to get used to a consistent naming scheme that uses components that, left to right, go from more general (data) to more specific (filtered).

Of course this comment applies to all converter functions.

I will update the variable names to follow a consistent naming scheme, using data, data_filtered, and data_validated, in the next PR to enhance code clarity and maintainability.

Why do you use WRROCData for the TES converted, but WRROCDataWES for the WES converter? Please use consistent naming.

And why do the converters for TES (filter irrelevant/extra fields) and WES (raise error for extra fields) behave so differently?

Maybe there is a good reason for the different behavior that I am missing. In that case, please explain. However, if there is no good reason, please apply a consistent behavior (probably the one used in TES).

I will align the behavior of TES and WES converters to ensure consistent handling of irrelevant/extra fields by filtering them out. Additionally, I will standardize the use of WRROCData and WRROCDataWES naming conventions.

See relevant comments in TES converter above.

I will address these comments in the next PR, applying the same changes to both TES and WES converters for consistency.

I don't understand why unexpected fields should raise an error? How do they bother you? It is quite likely that any real world (WR)ROC would raise an error here. The behavior in the TES converter makes much more sense: filter out what you don't need and then validate the remainder against what you need.

I agree. I will update the WES converter to filter out unexpected fields, similar to the TES converter, to handle real-world WRROC data more robustly.

When running the tool with WRROC data as input, you probably want to first validate that the input is indeed a valid WRROC entity. For that, you could use a model WRROCData (or just WRROC). This validation would be useful whenever you deal with WRROC data, e.g., in the convert_from_wrroc() methods of both the WES and TES converters. So you could consider writing an external WRROC validator function (perhaps in crategen.validators?) that you then import in the converters, so that you do not need to repeat code.

In a next step, you then want to validate that the WRROC data has all the fields required for the requested conversion. This code is specific to the converter, i.e., you may need different data for the conversion to TES than you need for the conversion to WES. For that you could define models WRROCDataTES and WRROCDataWES.

I will implement an external WRROC validator function to ensure WRROC data validity and avoid code repetition. Additionally, I will define separate models for WRROCDataTES and WRROCDataWES for specific validation in the respective converters.

When using Python 3.9 or newer (which I hope you do), please use list and dict instead of the uppercase variants List and Dict for type hints and Pydantic models.

See https://docs.python.org/3/whatsnew/3.9.html#type-hinting-generics-in-standard-collections

I will update the type hints to use list and dict instead of List and Dict to align with Python 3.9+ standards.

This doesn't currently seem to be used. If you want (or need) to include code to filter out additional properties not needed by WES/TES, you also need to put that code somewhere. Two options:

1. You add the code that validates the WRROC before you pass it to the appropriate handler (WES or TES). If you do the filtering, you want to include that logic in the specific handler (WES or TES) - or you put that logic in the validators themselves. I tend to prefer this option.
2. You call the validator within the validate_wrroc_wes() and validate_wrroc_tes() validators before you do the TES-/WES-specific validations. In that case any filtering logic, if necessary, needs to be also put in the specific validators, after the general WRROC validation and before the TES-/WES-specific validation.

If you do need to filter out additional properties, please try to abstract and write a function for that where you somehow pass the properties that you want to keep.

An additional note about the additional properties: I really don't think we need to filter those out - they don't hurt us. We should only validate what we need to validate, and that is that everything we need is available and of the correct/expected type.

Please work on the models first (see comments below and above), then rewrite your validators to account for what is written in those comments (especially the one below). But as I said, please do so in a clean, fresh PR that only changes/adds the 3 mentioned files. Make sure the code adheres to all good standards (docstrings, typing, error chaining etc), but only for the new code - the old code will be changed later.

Please double check your WRROCData, WRROCDataTES and WRROCDataWES models according to the mapping you did at the beginning of the project.

• We want to use WRROCData to validate any WRROC entity. However, given that there are three different profiles (Process Run, Workflow Run, Provenance Run), we want to have three different models. Luckily, given that these profiles are hierarchical (with Process Run being the least and Provenance Run being the most complex), we can make use of inheritance to avoid repeating ourselves. So you can start with the least complex one (WRROCProcess) and include all of its defined properties. None of its optional and all of its required properties should be required in your model. Additional properties should be allowed (see comment further below). Then go on with the next (WRROCWorkflow) and inherit from WRROCProcess. Only add additional properties defined for the Workflow Run profile that are not defined for Process Run (because the ones defined there will be inherited). If there are properties that are required for Workflow Run that were only optional for Process Run, override them. Finally, define WRROCProvenance in the same way, this time inheriting from WRROCWorkflow. Having these models, you can then write a validator that not only checks if a given object is a WRROC entity, but it will also tell you which profile(s) it adheres to. You can do so by first validating against WRROCProvenance. If it passes, you know that (1) the object is a WRROC entity and (2) it adheres to all profiles. If it does not pass, you validate against WRROCWorkflow. If that passes, you know that (1) the object is a WRROC entity and (2) it adheres to the Workflow Run and Process Run profiles. If it does not pass, you validate against WRROCProcess. If that passes, you know that (1) the object is a WRROC entity and (2) it adheres only to the Process Run profile. It it does not pass, you know that the object is not a WRROC entity. Having these models will be very useful to validate the input and also to automatically detect which conversions are possible.
• WRROCDataWES and WRROCDataTES should define ONLY those WRROC properties that are required to extract a WES and TES request from, respectively. We will use this to check whether we can proceed with the WRROC to WES conversion. Additional properties should be allowed (see comment further below). Given that both WRROCDataWES and WRROCDataTES are strict subsets of the core WRROC models above with respect to their properties (that is, e.g., all of the properties of WRROCDataWES will be defined somewhere in either of the WRROC profile models), you could try to avoid repetitions by using one of the approaches discussed here. However, if you do that, consider that some of the properties could be required for WRROCDataWES and WRROCDataTES, but are only optional in the WRROC profiles.

I think the models and the corresponding validators (including unit tests etc.) should be a single PR.