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

Updates to requirements packages and Elasticsearch connections #309

Closed
wants to merge 13 commits into from

Conversation

braswent
Copy link
Collaborator

@braswent braswent commented Aug 9, 2023

These are basic package updates to Dug. The Dug APIs in our project have been separated into it's own repository.
The biggest updates here are:

  • API interfacing with Elasticsearch has changed
  • Handling the updates to the data model with biolink.
  • Small bug fixes for dealing with error when using Roger

@braswent braswent requested a review from YaphetKG August 9, 2023 14:38
@braswent braswent self-assigned this Aug 9, 2023
@braswent braswent requested review from Hoid and vladimir2217 August 9, 2023 14:39
@@ -3,7 +3,7 @@
# A container for the core semantic-search capability.
#
######################################################
FROM python:3.9.6-slim
FROM python:3.10.10-slim
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we get this updated to 3.11?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I attempted to use 3.11 it caused additional breaking packages.

I will try again now that you are using fastAPI instead of flask though if I remember right that was the issue however it may also cause issues with Roger.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@braswent if it takes more than minor updates, i think it will be enough to create a task for this and handle it separately.

app.logger.error (f"ERROR: {str(error)}")
traceback.print_exc (error)
abort(Response(str(error), 400))
# if not self.specs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is all due to issues with the JSON from elasticsearch not playing with the old flask api conversion to a dictionary. This is apparently a issue that is known but as of yet not solved however we do not need to worry about it since we have moved to FastAPI anyways

Copy link
Collaborator

@vladimir2217 vladimir2217 left a comment

Choose a reason for hiding this comment

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

Please find comments

description = Digging up data
long_description = file: README.md
long_description_content_type = text/markdown
url = https://github.com/helxplatform/dug
url = https://github.com/RTIInternational/dug
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to revert this back to this repo ?

Copy link
Contributor

Choose a reason for hiding this comment

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

since we have converted to fastapi , i don't think we should include api.py in the codebase anymore, maybe the elastic search query object needs these changes in the fastapi (server.py ) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah the server.py has to change. I can work on that since I did that already in the Dug-API repo I created

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need the scheme ? is this for support for HTTPS?
Wondering if we need this to be in roger config too ? maybe we can specify it on the host itself ?

node_type = [self._snake_case(x.replace('biolink:', '')) for x in node_details.get("category", [])]
category = node_details.get("category", [])
if type(category) == list:
node_type = [self._snake_case(x.replace('biolink.', '')) for x in category]
Copy link
Contributor

Choose a reason for hiding this comment

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

can we store the biolink. in the config object somewhere and pass it down ?
I think that way we could use this code with the current graph, and later change the config and would be able to run it on multiple labeled graph aswell ?

@@ -1,4 +1,4 @@
elasticsearch==7.12.0
elasticsearch==8.5.2
flake8
flasgger
Flask
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are not using Flask anymore, could you delete this entry?

@YaphetKG YaphetKG changed the base branch from develop to nathan-change September 26, 2023 20:51
@YaphetKG
Copy link
Contributor

merged via #322

@YaphetKG YaphetKG closed this Oct 27, 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.

3 participants