Skip to content
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

Update Migration Guide #420

Merged
merged 12 commits into from
May 18, 2023
Merged

Update Migration Guide #420

merged 12 commits into from
May 18, 2023

Conversation

uhbrar
Copy link
Contributor

@uhbrar uhbrar commented Apr 4, 2023

This adds a guide dedicated more to KP's as opposed to ARA's. This also includes an example for source retrieval provenance refactoring.

@edeutsch
Copy link
Collaborator

edeutsch commented Apr 4, 2023

You can more easily read the new formatted guide here: https://github.com/uhbrar/ReasonerAPI/blob/update_guide/MigrationAndImplementationGuide1-4.md

Copy link
Collaborator

@edeutsch edeutsch left a comment

Choose a reason for hiding this comment

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

There seemed to be some substantial push-back against implementing this policy when we brought this up, so I think this will need some more discussion before merging.

}
]
},
"edge_bindings": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think results have edge_bindings any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was decided that KP's should not use result.edge_bindings and should instead construct an analysis object.

@uhbrar
Copy link
Contributor Author

uhbrar commented Apr 7, 2023

I thought that it was pretty well accepted that subclasses could be represented this way when discussed at architecture. There was definitely discussion about this use case being added to the guide.

@edeutsch
Copy link
Collaborator

edeutsch commented Apr 7, 2023

My recollection from the TRAPI call is that this was discussed and there was substantial push-back, perhaps from Vlado and others, I don't recall now exactly. The notes say:

There was mention of KPs potentially using AuxiliaryGraph for subclass encoding? Do we want to pursue that? Come up with a recommendation/example on how to encode a subclass reasoning response from a KP? Point people: Chris, Abrar, Matt, Andrew?
For now, the policy is to just keep doing it the current way, and we will consider a change in the future
[Eric] will start a thread to discuss if it is sensible to create a subclass example. DONE

I started the issue here: #414
There is some discussion, but I suspect others will want to contribute.

I think this change will need formal approvals before merging.
There are multiple revisions in this PR so it is hard to see the specific proposed changed, but this link may be more helpful:
e9c41d5

I suggest conversion continue in the issue.
And the topic could be revisited in the Architecture and/or TRAPI calls next week.

@@ -1,10 +1,12 @@
# TRAPI 1.4 Migration and Implementation Guide
This guide is split into two major sections. The first section focusses on ARA's. The second section focusses on KP's.

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -225,7 +334,7 @@ Most of the knowledge graph will remain the same in this example. Only one of th
"predicate": "treats",
"attributes": [
{
"attribute_type_id": "support graph",
"attribute_type_id": "infores:support_graphs",
Copy link
Contributor

@colleenXu colleenXu Apr 11, 2023

Choose a reason for hiding this comment

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

is this supposed to be "biolink:support_graphs", from here?

And is the value for this attribute_type_id always an array (vs a string key for one auxiliary graph)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thank you.

There's noting in the specification that necessarily says that it has to be a list, but I think expected practice is a list as opposed to a single value. Unless this was further specified in the biolink model.

@@ -452,7 +569,7 @@ Finally, we can put all of it back together, to show the full message.
"predicate": "treats",
"attributes": [
{
"attribute_type_id": "support graph",
"attribute_type_id": "infores:support_graphs",

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

"query_graph": {
"nodes": {
"query_node_0": {
"ids": ["MONDO:0005148"]

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thank you.

@colleenXu
Copy link
Contributor

Also does the description for Result need editing since it discusses edges and edge-bindings? https://github.com/uhbrar/ReasonerAPI/blob/1.4/TranslatorReasonerAPI.yaml#L564

@uhbrar
Copy link
Contributor Author

uhbrar commented Apr 11, 2023

I didn't notice that. I thought I'd already fixed the description for Result. I'll put in a separate pull request in for that.

@@ -1,10 +1,12 @@
# TRAPI 1.4 Migration and Implementation Guide
This guide is split into two major sections. The first section focuses on ARAs. The second section focusses on KPs.
Copy link
Contributor

Choose a reason for hiding this comment

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

still has a "focusses" typo

@edeutsch edeutsch added this to the v1.4 milestone Apr 27, 2023
@uhbrar uhbrar requested a review from edeutsch May 1, 2023 20:12
@edeutsch
Copy link
Collaborator

The number of approvals is very meagre, but I'm going to merge this anyway, because if we actually wait for approvals we'll never merge it.

@edeutsch edeutsch merged commit c4d2f24 into NCATSTranslator:1.4 May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants