-
Notifications
You must be signed in to change notification settings - Fork 1
Pathfinder test runner #34
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.
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
Conversation
b0a6f94 to
9a54506
Compare
test_harness/reporter.py
Outdated
| }, | ||
| { | ||
| "key": "PathOutputCuries", | ||
| "value": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All keys and values need to be strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this part of the label
| found_path_nodes.add(edge["object"]) | ||
| if len(found_path_nodes) > 0: | ||
| report[agent]["status"] = "PASSED" | ||
| report[agent]["expected_path_nodes"] = found_path_nodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Everything being uploaded to the Information Radiator needs to be a string.
|
Based on our offline conversation from yesterday, here's what I think should happen/how it should be laid out:
|
|
The pathfinder test runner has been updated to be more consistent with the way tests are run for existing test runners. We now include a minimum required path nodes as part of assets to determine how many nodes must exist within a path for the test to pass. |
| except Exception as e: | ||
| logger.warning(f"Failed to run TRAPI validation with {e}") | ||
| agent_report["trapi_validation"] = "ERROR" | ||
| # try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also take out where report["trapi_validation"] gets set to "NA" as well: https://github.com/TranslatorSRI/TestHarness/pull/34/files#diff-0621f2a4d304860a26d6a03485a9f4301d0bde4bbc98843167bd41cb3aca3b03R101
| matching_path_nodes.add(edge["object"]) | ||
| if len(matching_path_nodes) >= minimum_required_path_nodes: | ||
| found_path_nodes.add(",".join(matching_path_nodes)) | ||
| if len(found_path_nodes) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple things here:
- I know we talked about including the expected intermediate nodes in the test labels, and I think it's right that they aren't included there, but it would be nice if they showed up somewhere, probably in the report. Otherwise, the test doesn't show what they are anywhere. Creative queries are easier because the title includes the expected answer nodes.
- In the same vein, maybe also including the minimum number of required path nodes would be useful
- I'm not sure it helps anyone to include the
expected_path_nodesif someone passed the test? Let's rethink what all should be included in the report so that someone could get just about all the information they need just by looking at the test output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's true. So I guess the best thing to do would be to include the minimum required path node number, as well as the expected intermediate nodes, regardless of whether or not the test is passed?
I do think providing the expected_path_nodes is useful even if the test is passed. That way the test output makes clear exactly how the test is passed, by showing what pairs are actually found. Maybe the name should be changed? expected_paths_found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all sounds good. I would maybe even include the expected_nodes_found(note the updated name) for failed tests, to show how close/far they were from passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of including the minimum number and expected intermediate nodes for every ARA, you could include it as a higher field in the whole report. Here's an example of the full report output:
{
"pks": {
"parent_pk": "cc8f8ac6-4582-4959-b96a-bb44e92fbf0e",
"improving-agent": "0aeadcb7-3407-456c-8d2a-721fba2f8860",
"molepro": "0b582315-48ac-4092-b666-d9bf2a57ddf6",
"multiomics-drugapprovals": "19bc999b-1041-4b78-95c3-c18528d25fc9",
"connections-hypothesis": "212f904b-ae03-46ea-9779-6f69aa5ac8d3",
"unsecret-agent": "27f4f2b8-11bd-4d24-8aa7-cd73959438f5",
"cohd": "2c53bc0f-a5e5-41ac-b278-3e2d765b3ee5",
"genetics-data-provider": "301d50d0-587f-4c6f-ac55-4682501f0c37",
"aragorn": "3e0e9faf-4a2a-479e-9e6f-ef7327164c98",
"biothings-explorer": "aee66e2d-9c88-4f42-80a2-c11a30f4f651",
"automat-cam-kp": "bc727dcf-9487-40a8-8b33-65805094a051",
"text-mining-provider-targeted": "c3abf709-a8d3-4dc6-895e-24c3715ec2f4",
"arax": "dc64651d-08f2-4f6a-90a1-83f5127bca42",
"multiomics-clinicaltrials": "ddd4c246-424a-4bb7-a4c8-e0bc38c9578b",
"openpredict": "f0e177c2-b878-4b39-be50-6ea57cb61595",
"ars": "9c6fb07c-c9cb-49c8-8333-016193b66bb4"
},
"result": {
"improving-agent": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 400"
},
"molepro": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 400"
},
"multiomics-drugapprovals": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 400"
},
"connections-hypothesis": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 400"
},
"unsecret-agent": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 503"
},
"cohd": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 400"
},
"genetics-data-provider": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 400"
},
"aragorn": {
"trapi_validation": "NA",
"status": "FAILED"
},
"biothings-explorer": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 400"
},
"automat-cam-kp": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 422"
},
"text-mining-provider-targeted": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 400"
},
"arax": {
"trapi_validation": "NA",
"status": "PASSED",
"expected_path_nodes": "NCBIGene:3815,CL:0000097"
},
"multiomics-clinicaltrials": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 400"
},
"openpredict": {
"trapi_validation": "NA",
"status": "FAILED",
"message": "Status code: 502"
},
"ars": {
"trapi_validation": "NA",
"status": "PASSED",
"expected_path_nodes": "NCBIGene:3815,CL:0000097"
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added both of these outside of the individual agent reports, and also added more details to the agent reports as well.
test_harness/runner/query_runner.py
Outdated
| return responses, pks | ||
|
|
||
|
|
||
| class QueryRunner(BaseQueryRunner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is leftover from something else? I don't think we need all this split out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was a vestigial structure from my initial implementation, which was to rewrite everything. I guess I forgot to get rid of this.
maximusunc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, and runs successfully!
Adds test runner for pathfinder queries and integration into test harness