Skip to content

test(tes,wes): add comprehensive unit tests #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Karanjot786 wants to merge 6 commits into
base: main
Choose a base branch
from
Open

test(tes,wes): add comprehensive unit tests #17

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
de59af2
test: add comprehensive unit tests for TES and WES converters
Karanjot786 Jul 26, 2024
879a84a
test: add comprehensive unit tests for TES and WES converters
Karanjot786 Jul 27, 2024
ddaa0de
test(tes,wes): add comprehensive unit tests
Karanjot786 Aug 1, 2024
75ee3a5
test(tes,wes): add comprehensive unit tests
Karanjot786 Aug 5, 2024
29f774d
test(tes,wes): add comprehensive unit tests
Karanjot786 Aug 6, 2024
1300eb3
test(tes,wes): add comprehensive unit tests
Karanjot786 Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:

- name: Lint with Ruff
run: |
poetry run ruff check crategen/
poetry run ruff check crategen/ tests/

- name: Type check with Mypy
run: |
Expand All @@ -39,6 +39,6 @@ jobs:
run: |
poetry add pytest pytest-cov pytest-mock

# - name: Run tests
# run: |
# poetry run pytest --cov=crategen
- name: Run tests
run: |
poetry run pytest --cov=crategen
62 changes: 26 additions & 36 deletions crategen/converters/tes_converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,42 @@
from pydantic import ValidationError
from .abstract_converter import AbstractConverter
from .utils import convert_to_iso8601
from ..models import TESData, WRROCData

class TESConverter(AbstractConverter):

def convert_to_wrroc(self, tes_data):
# Validate and extract data with defaults
id = tes_data.get("id", "")
name = tes_data.get("name", "")
description = tes_data.get("description", "")
executors = tes_data.get("executors", [{}])
inputs = tes_data.get("inputs", [])
outputs = tes_data.get("outputs", [])
creation_time = tes_data.get("creation_time", "")
end_time = tes_data.get("logs", [{}])[0].get("end_time", "") # Corrected to fetch from logs
try:
validated_tes_data = TESData(**tes_data)
except ValidationError as e:
raise ValueError(f"Invalid TES data: {e}")

# Convert to WRROC
wrroc_data = {
"@id": id,
"name": name,
"description": description,
"instrument": executors[0].get("image", None) if executors else None,
"object": [{"@id": input.get("url", ""), "name": input.get("path", "")} for input in inputs],
"result": [{"@id": output.get("url", ""), "name": output.get("path", "")} for output in outputs],
"startTime": convert_to_iso8601(creation_time),
"endTime": convert_to_iso8601(end_time),
"@id": validated_tes_data.id,
"name": validated_tes_data.name,
"description": validated_tes_data.description,
"instrument": validated_tes_data.executors[0].image if validated_tes_data.executors else None,
"object": [{"@id": input.url, "name": input.path} for input in validated_tes_data.inputs],
"result": [{"@id": output.url, "name": output.path} for output in validated_tes_data.outputs],
"startTime": convert_to_iso8601(validated_tes_data.creation_time),
"endTime": convert_to_iso8601(validated_tes_data.logs[0].end_time) if validated_tes_data.logs else None,
}
return wrroc_data

def convert_from_wrroc(self, wrroc_data):
# Validate and extract data with defaults
id = wrroc_data.get("@id", "")
name = wrroc_data.get("name", "")
description = wrroc_data.get("description", "")
instrument = wrroc_data.get("instrument", "")
object_data = wrroc_data.get("object", [])
result_data = wrroc_data.get("result", [])
start_time = wrroc_data.get("startTime", "")
end_time = wrroc_data.get("endTime", "")
try:
validated_wrroc_data = WRROCData(**wrroc_data)
except ValidationError as e:
raise ValueError(f"Invalid WRROC data: {e}")
Copy link
Copy Markdown
Member

@uniqueg uniqueg Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member Author

@Karanjot786 Karanjot786 Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@uniqueg uniqueg Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# Convert from WRROC to TES
tes_data = {
"id": id,
"name": name,
"description": description,
"executors": [{"image": instrument}],
"inputs": [{"url": obj.get("@id", ""), "path": obj.get("name", "")} for obj in object_data],
"outputs": [{"url": res.get("@id", ""), "path": res.get("name", "")} for res in result_data],
"creation_time": start_time,
"logs": [{"end_time": end_time}], # Added to logs
"id": validated_wrroc_data.id,
"name": validated_wrroc_data.name,
"description": validated_wrroc_data.description,
"executors": [{"image": validated_wrroc_data.instrument}],
"inputs": [{"url": obj.id, "path": obj.name} for obj in validated_wrroc_data.object],
"outputs": [{"url": res.id, "path": res.name} for res in validated_wrroc_data.result],
"creation_time": validated_wrroc_data.startTime,
"logs": [{"end_time": validated_wrroc_data.endTime}],
}
return tes_data
55 changes: 26 additions & 29 deletions crategen/converters/wes_converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,43 @@
from pydantic import ValidationError
from .abstract_converter import AbstractConverter
from ..models import WESData, WRROCDataWES
from .utils import convert_to_iso8601

class WESConverter(AbstractConverter):

def convert_to_wrroc(self, wes_data):
# Validate and extract data with defaults
run_id = wes_data.get("run_id", "")
name = wes_data.get("run_log", {}).get("name", "")
state = wes_data.get("state", "")
start_time = wes_data.get("run_log", {}).get("start_time", "")
end_time = wes_data.get("run_log", {}).get("end_time", "")
outputs = wes_data.get("outputs", {})
try:
wes_model = WESData(**wes_data)
except ValidationError as e:
raise ValueError(f"Invalid WES data: {e}")
outputs = wes_model.outputs

# Convert to WRROC
wrroc_data = {
"@id": run_id,
"name": name,
"status": state,
"startTime": convert_to_iso8601(start_time),
"endTime": convert_to_iso8601(end_time),
"result": [{"@id": output.get("location", ""), "name": output.get("name", "")} for output in outputs],
"@id": wes_model.run_id,
"name": wes_model.run_log.name,
"status": wes_model.state,
"startTime": convert_to_iso8601(wes_model.run_log.start_time),
"endTime": convert_to_iso8601(wes_model.run_log.end_time),
"result": [{"@id": output.location, "name": output.name} for output in outputs],
}
return wrroc_data

def convert_from_wrroc(self, wrroc_data):
# Validate and extract data with defaults
run_id = wrroc_data.get("@id", "")
name = wrroc_data.get("name", "")
start_time = wrroc_data.get("startTime", "")
end_time = wrroc_data.get("endTime", "")
state = wrroc_data.get("status", "")
result_data = wrroc_data.get("result", [])

# Convert from WRROC to WES
try:
wrroc_model = WRROCDataWES(**wrroc_data)
except ValidationError as e:
raise ValueError(f"Invalid WRROC data: {e}")

result_data = wrroc_model.result

wes_data = {
"run_id": run_id,
"run_id": wrroc_model.id,
"run_log": {
"name": name,
"start_time": start_time,
"end_time": end_time,
"name": wrroc_model.name,
"start_time": wrroc_model.startTime,
"end_time": wrroc_model.endTime,
},
"state": state,
"outputs": [{"location": res.get("@id", ""), "name": res.get("name", "")} for res in result_data],
"state": wrroc_model.status,
"outputs": [{"location": res.id, "name": res.name} for res in result_data],
}
return wes_data
96 changes: 96 additions & 0 deletions crategen/models.py
View file
Open in desktop
Copy link
Copy Markdown
Member

@uniqueg uniqueg Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@Karanjot786 Karanjot786 Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@uniqueg uniqueg Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
from pydantic import BaseModel, Field, validator,root_validator
from typing import List, Optional, Dict, Any

class Executor(BaseModel):
image: str
command: List[str]

class TESInputs(BaseModel):
url: str
path: str

class TESOutputs(BaseModel):
url: str
path: str

class TESLogs(BaseModel):
end_time: Optional[str] = None

class TESData(BaseModel):
id: str
name: str
description: Optional[str] = ""
executors: List[Executor]
inputs: List[TESInputs]
outputs: List[TESOutputs]
creation_time: str
logs: List[TESLogs]

class WESRunLog(BaseModel):
name: Optional[str] = None
start_time: Optional[str] = None
end_time: Optional[str] = None

class WESOutputs(BaseModel):
location: str
name: str

class WESData(BaseModel):
run_id: str
run_log: WESRunLog
state: str
outputs: List[WESOutputs]

@root_validator(pre=True)
Copy link
Copy Markdown
Contributor

@salihudickson salihudickson Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@Karanjot786 Karanjot786 Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def check_unexpected_fields(cls, values):
allowed_fields = {"run_id", "run_log", "state", "outputs"}
unexpected = set(values.keys()) - allowed_fields
if unexpected:
raise ValueError(f"Unexpected fields: {unexpected}")
return values

class WRROCInputs(BaseModel):
id: str
name: str

class WRROCOutputs(BaseModel):
id: str
name: str

class WRROCData(BaseModel):
id: str
name: str
description: Optional[str] = ""
instrument: Optional[str] = None
object: List[WRROCInputs]
result: List[WRROCOutputs]
startTime: Optional[str] = None
endTime: Optional[str] = None

@validator('id')
Copy link
Copy Markdown
Contributor

@salihudickson salihudickson Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def id_must_be_string(cls, value):
if not isinstance(value, str):
raise ValueError('Invalid id type')
return value

@validator('name')
def name_must_be_string(cls, value):
if not isinstance(value, str):
raise ValueError('Invalid name type')
return value

class WRROCDataWES(BaseModel):
id: str
name: str
status: str
result: List[WRROCOutputs]
startTime: Optional[str] = None
endTime: Optional[str] = None

@root_validator(pre=True)
Copy link
Copy Markdown
Contributor

@salihudickson salihudickson Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment for the WES Data Model applies here as well

def check_unexpected_fields(cls, values):
allowed_fields = {"id", "name", "startTime", "endTime", "status", "result"}
unexpected = set(values.keys()) - allowed_fields
if unexpected:
raise ValueError(f"Unexpected fields: {unexpected}")
return values
Loading