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

Fix missing node bindings in NodeNorm /query output #231

Merged
merged 5 commits into from
Dec 7, 2023
Merged

Conversation

gaurav
Copy link
Contributor

@gaurav gaurav commented Nov 27, 2023

As reported in #229. As far as I can tell, this was caused by merged_binding being None, which could cause it to end up being an object later in the code. Handling the case where it's None appears to fix this problem as per the simple test in PR TranslatorSRI/babel-validation#26.

Fixes #229.

@gaurav gaurav mentioned this pull request Nov 27, 2023
@gaurav gaurav marked this pull request as ready for review November 27, 2023 19:34
@gaurav gaurav requested a review from cbizon November 27, 2023 19:34
Copy link
Contributor

@cbizon cbizon left a comment

Choose a reason for hiding this comment

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

Is that localhost entry in the openapi going to make it into the version hosted at the smartapi registry?

@gaurav gaurav requested a review from YaphetKG November 28, 2023 00:36
@gaurav
Copy link
Contributor Author

gaurav commented Nov 28, 2023

NameRes uses the same servers setup, so we can use that see what happens.

Bad news: the localhost entries do get picked up by SmartAPI via Yaphet's trapi-openapi app: https://trapi-openapi.apps.renci.org/utility/infores%3Asri-name-resolver?version=%2A

Good news: those localhost entries somehow gets magically turned into the current server name on https://name-resolution-sri-dev.apps.renci.org/docs, https://name-resolution-sri.renci.org/docs and https://name-lookup.ci.transltr.io/docs and in the trapi-openapi app. So the server does gets duplicated, but localhost:2433 never itself gets added. So it should work the same for NodeNorm.

I'm going to ask @YaphetKG to review this PR as well in case there's a better way to do what I'm trying to do here.

@gaurav
Copy link
Contributor Author

gaurav commented Nov 29, 2023

I should have added in my previous comment that I've deployed this to https://nodenormalization-dev.apps.renci.org/docs and that does correctly transform localhost to the URL (i.e. https://nodenormalization-dev.apps.renci.org/1.4). So that localhost entry should be fine.

@cbizon cbizon self-requested a review December 4, 2023 14:16
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.

/query Removing analyses
2 participants