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

Check Qnode/Qedge properties #125

Merged
merged 8 commits into from
Jan 6, 2023
Merged

Check Qnode/Qedge properties #125

merged 8 commits into from
Jan 6, 2023

Conversation

rjawesome
Copy link
Contributor

Solves issue

@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #125 (7016e3e) into main (add1e58) will decrease coverage by 6.42%.
The diff coverage is 92.30%.

❗ Current head 7016e3e differs from pull request most recent head ab76c46. Consider uploading reports for the commit ab76c46 to get more accurate results

@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
- Coverage   59.91%   53.48%   -6.43%     
==========================================
  Files          25       26       +1     
  Lines        2365     2307      -58     
==========================================
- Hits         1417     1234     -183     
- Misses        948     1073     +125     
Impacted Files Coverage Δ
src/inferred_mode/inferred_mode.js 2.62% <0.00%> (-61.48%) ⬇️
src/index.js 62.91% <66.66%> (-6.22%) ⬇️
src/query_graph.js 69.36% <100.00%> (+4.54%) ⬆️
src/cache_handler.js 16.42% <0.00%> (-66.31%) ⬇️
src/inferred_mode/template_lookup.js 14.28% <0.00%> (-57.72%) ⬇️
src/utils.js 48.27% <0.00%> (-51.73%) ⬇️
src/redis-client.js 47.94% <0.00%> (-8.22%) ⬇️
src/batch_edge_query.js 83.72% <0.00%> (-0.19%) ⬇️
... and 13 more

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

@colleenXu
Copy link
Contributor

colleenXu commented Sep 23, 2022

looks like QEdge is missing some specified fields? like knowledge_type, attribute_constraints, qualifier_constraints (doesn't use constraints)

@colleenXu
Copy link
Contributor

colleenXu commented Oct 4, 2022

@tokebe

@andrewsu and I wondered if the TRAPI spec could be used, rather than hard-coded constants. BTE stores a modified TRAPI spec locally and it has the sections for QNode and QEdge

Note that I haven't tested this PR; I've only commented after looking at the files changed...

EDIT: linking the requirements in the original issue biothings/biothings_explorer#503.

@tokebe
Copy link
Member

tokebe commented Oct 4, 2022

Agreed, it would probably be best to dynamically check using the TRAPI spec rather than using hard-coded values.

@rjawesome
Copy link
Contributor Author

Wrote some code to use TRAPI Spec, required some code in the main BTE repo as well, will push soon under same branch name (may break tests due to changes in TRAPIQueryHandler constructor, will address soon).

Also, while testing I found out that for some reason QEdge.constraints was being used in the code even thought that isn't in the TRAPI spec? (I got an error when making it to a string)

@colleenXu
Copy link
Contributor

colleenXu commented Oct 6, 2022

The previous QEdge.constraints work is likely from this stuff biothings/biothings_explorer#302, biothings/biothings_explorer#174 (comment). I think it is currently broken, and the TRAPI spec changed it from QEdge.constraints to QEdge.attribute_constraints.

Also, I think @tokebe would be the one to comment on whether the new commits / PR biothings/biothings_explorer#513 make sense (I'm a bit surprised that the whole openapi spec is parsed).

If it seems fine, then someone could go forward with testing this set of PRs to see if they solve the issue. I wonder if anyone feels comfortable handling this? Or it can be put in the pile for me to test later :P

@tokebe
Copy link
Member

tokebe commented Oct 6, 2022

@colleenXu the spec is parsed once during startup and then held in memory for query validation using swagger.

@rjawesome If you could edit your new PR to only read/parse the spec once (perhaps when utils is imported or something similar) that would be better. Reading/parsing the whole thing for each query would probably add a not-insignificant delay and memory overhead to handling requests.

@tokebe
Copy link
Member

tokebe commented Oct 6, 2022

Also @colleenXu I can handle testing when it's ready.

@tokebe
Copy link
Member

tokebe commented Oct 10, 2022

@rjawesome Based on your changes this looks good -- if you think it's ready now then I'll do some quick testing and merge to dev for final testing.

One nitpick prior to that, however: I'd prefer BTE's logs to be less "human" to fit with the general tone of the rest of our logs (We've encountered X -> Encountered X/We will ignore X -> X will be ignored, along those lines).

@rjawesome
Copy link
Contributor Author

Yeah I believe it's ready, I can change the logs if needed.

@tokebe
Copy link
Member

tokebe commented Oct 11, 2022

Please do, I'll get testing in the meantime.

@rjawesome
Copy link
Contributor Author

Logs fixed

@tokebe tokebe merged commit e65e2a2 into main Jan 6, 2023
@tokebe tokebe deleted the property-warning branch November 29, 2023 19:08
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.

3 participants