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

Use JQ #38

Merged
merged 59 commits into from
Oct 24, 2023
Merged

Use JQ #38

merged 59 commits into from
Oct 24, 2023

Conversation

rjawesome
Copy link
Contributor

@rjawesome rjawesome commented Sep 21, 2022

Draft PR

Depends on installation of node-jq. Currently tested with a basic PR of BioLink shown below (would be useful to have a request with publications in the output).

{
    "message": {
        "query_graph": {
            "nodes": {
                "n0": {
                    "categories": ["biolink:Pathway"]
                },
                "n1": {
                    "ids": ["MONDO:0008974"],
                    "categories": ["biolink:Disease"]
                }
            },
            "edges": {
                "e0": {
                    "subject": "n1",
                    "object": "n0"
                }
            }
        }
    }
}

For this issue

@rjawesome rjawesome marked this pull request as draft September 21, 2022 00:48
@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Merging #38 (95743bf) into main (85bba2a) will decrease coverage by 1.62%.
Report is 22 commits behind head on main.
The diff coverage is 86.95%.

❗ Current head 95743bf differs from pull request most recent head 17496d8. Consider uploading reports for the commit 17496d8 to get more accurate results

@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
- Coverage   63.19%   61.57%   -1.62%     
==========================================
  Files          11       10       -1     
  Lines         394      380      -14     
  Branches       91       86       -5     
==========================================
- Hits          249      234      -15     
+ Misses        114      111       -3     
- Partials       31       35       +4     
Files Coverage Δ
src/jq_utils.ts 100.00% <100.00%> (ø)
src/transformers/biothings_transformer.ts 62.50% <ø> (-25.00%) ⬇️
src/transformers/transformer.ts 87.32% <100.00%> (ø)
src/transformers/jq_transfomer.ts 76.92% <76.92%> (ø)

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@colleenXu
Copy link
Contributor

For Biolink API and most other APIs, there are comments for each operation with examples (ID to use as input, categories and predicates to use):

This operation should return publication info:


{
    "message": {
        "query_graph": {
            "nodes": {
                "n0": {
                    "categories": ["biolink:SequenceVariant"],
                    "ids": ["DBSNP:rs227731", "DBSNP:rs2230199"]
                },
                "n1": {
                    "categories": ["biolink:PhenotypicFeature"]
                }
            },
            "edges": {
                "e01": {
                    "subject": "n0",
                    "object": "n1"
                }
            }
        }
    }
}

@newgene
Copy link
Member

newgene commented Oct 4, 2022

@rjawesome to further expand this feature, can you please create a place-holder js module, where we can define some common custom jq helper functions, which can be used in jq string.

@rjawesome
Copy link
Contributor Author

Notes: ebi test query works, biolink api publications works

@newgene
Copy link
Member

newgene commented Oct 25, 2022

@rjawesome refactoring looks good to me. In the context of the new issue I just created (since we moved to jq instead of JMESPath), I think there two things for you to figure out:

  • a place-holder js module, where we can define some common custom jq helper functions, which can be used in jq string. For now, you can just put a dummy_func as a place-holder to demonstrate this process works.
  • How to pass the jq string from an API's SmartAPI metadata to this api-repone-transform.js module

@newgene
Copy link
Member

newgene commented Oct 26, 2022

@rjawesome from the discussion at today's translator team meeting, I think it makes sense to complete this JQ feature in two stages:

  1. Replacing the existing custom js-based API transformers into generic JQ-based API transformer.

    This is basically what this PR for. I think you may need to add (update the existing ones) some tests to this new feature. Then we can call it done of this PR and issue 489 and merge it, then test it with the actual queries on our dev server.

  2. Allow passing jq string from the SmartAPI metadata (basically this new issue #251)

    You can create a new PR to address this issue as the next step after completion of step 1.

@rjawesome
Copy link
Contributor Author

rjawesome commented Oct 28, 2022

Mostly done with 1 but need to fix the ebi jq string (cuirrently failing tests).

@rjawesome
Copy link
Contributor Author

Alright Stage 1 should be done.

@rjawesome
Copy link
Contributor Author

Work for Stage 2 started on this pr, on bte-trapi, and on smartapi-kg (all in the use-jmes-path branch)

@rjawesome
Copy link
Contributor Author

rjawesome commented Nov 12, 2022

Note (for me): May want to add standardized variables for edge info that can be used in JQ. For example semmedb transformer needs this.edge.association.output_type

@colleenXu
Copy link
Contributor

@tokebe

Note that this branch isn't merged with the latest main branch code. Is it trivial to make this update (doesn't break any functionality)?

I didn't notice any issues when I merged w/ latest main for my local branch with testing biothings/biothings_explorer#489 (comment). I did this because I noticed the ref_pmid field wasn't being used as I expected (aka the code to handle this is on the main branch and not currently in this branch).

@colleenXu

This comment was marked as outdated.

@colleenXu

This comment was marked as outdated.

@tokebe
Copy link
Member

tokebe commented Oct 13, 2023

I wasn't able to replicate the exact errors you had, however I also saw odd behavior when I went back to check. style: indent 2 Only changed code formatting, not functionality, so that can't be it. It looks like the code changes for using transformer.jq.wrap hadn't been correctly implemented in the smartapi-kg package, leading to the transformer becoming undefined. I've pushed a fix that should fix this behavior.

NOTES:

  • Make sure you pull the latest both in this and the smartapi-kg packages, as both PRs are required and the fix I pushed has changes in both
  • Due to current limitations in BTE's $ref resolution, you can only use a $ref for jq transformers like the following:
transformer:
  "$ref": "#/components/x-bte-transformers/whole"

The other types (from-wrap, from-string) won't resolve properly.

@colleenXu
Copy link
Contributor

colleenXu commented Oct 17, 2023

I hid the comments above, because I realized that I made a silly mistake in my testing....forgot the opening-quotation-mark so I was doing $ref" when I should have been doing "$ref"

On retesting "JQ in SmartAPI" with the correct "$ref" statements, I'm finding that Jackson's post above is correct:

  • now $ref for the entire contents of the operation's transformer field works
  • the other $ref scenarios don't work. They have the same behavior as before (jq-string not working or error statements)

I've put the "JQ in SmartAPI" yamls that I tested here. They worked w/ the basic JQ-work PRs (this and biothings/smartapi-kg.js#61).

@tokebe
Copy link
Member

tokebe commented Oct 23, 2023

Note: I've pushed a commit just applying a formatting, this'll make merging easier with the typescript/build system migration (either direction).

@tokebe tokebe merged commit b4188d3 into main Oct 24, 2023
0 of 2 checks passed
@tokebe tokebe deleted the use-jmes-path branch November 3, 2023 20:29
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.

4 participants