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

Feature/grip #36

Merged
merged 23 commits into from
Sep 17, 2024
Merged

Feature/grip #36

merged 23 commits into from
Sep 17, 2024

Conversation

matthewpeterkort
Copy link
Collaborator

@matthewpeterkort matthewpeterkort commented Aug 15, 2024

Adds grip etl functions to etl pod.

checkout ACED-IDP/gen3-helm#62 branch, make update, make local.

kc get services -- If local-grip doesn't appear in the list you might have to expose the service with
kc expose deployment local-grip

also containerizing fhir-server is a part of the helm PR.

The server doesn't automatically make the graph for you on a a graph that doesn't yet exist in grip. So currently you have to exec into the pod and make the graph with a grip create [graph-name] Should this be added ?

update your sower image to feature_grip , and then you should be able to load data with the util into grip.

check grip logs.

To verify that etl has worked, exec into grip pod,
and do a query like grip query [graph-name] 'V()' to get all of the vertices in the graph.

Recent commits have attached grip ETL to feed data directly into the data framer

@matthewpeterkort matthewpeterkort changed the base branch from development to feature/demo August 15, 2024 18:00
etl-job/Dockerfile Outdated Show resolved Hide resolved

RUN git clone https://github.com/bmeg/iceberg.git && \
cd iceberg && \
git checkout feature/FHIR-resource-type

Choose a reason for hiding this comment

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

similarly, do we expect this to be a long-lived branch or just for testing purposes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Walsh needs to approve this PR before it gets merged.

Choose a reason for hiding this comment

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

sounds good, up to you whether you wanna wait for it or patch it later

etl-job/fhir_import_export.py Outdated Show resolved Hide resolved
etl-job/fhir_import_export.py Outdated Show resolved Hide resolved
etl-job/fhir_import_export.py Outdated Show resolved Hide resolved

RUN git clone https://github.com/bmeg/iceberg.git && \
cd iceberg && \
git checkout feature/FHIR-resource-type

Choose a reason for hiding this comment

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

sounds good, up to you whether you wanna wait for it or patch it later

Base automatically changed from feature/demo to development September 9, 2024 20:10
@@ -357,21 +314,17 @@ def _empty_project(output: list[str],
"""Clear out graph and flat metadata for project """
# check permissions
try:
empty_project(program=program, project=project, dictionary_path=dictionary_path, config_path=config_path)
grip_delete(graph_name="CALIPER", project_id=f"{program}-{project}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should grip's URL and PORT be passed as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems the pod has the best information (it's env) to determine grip url, port etc.
Should we pass that information to the grip submission layer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes thinking about this further, need to add this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link

@quinnwai quinnwai left a comment

Choose a reason for hiding this comment

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

one question about partial job completion


index_generator_dict = {
'researchsubject': db.flattened_research_subjects,
'specimen': db.flattened_specimens,
'file': db.flattened_document_references
}

# To ensure differences in the dataframer versions do not conflict, clear the project, and reload the project.
for index in index_generator_dict.keys():
meta_flat_delete(project_id=f"{program}-{project}", index=index)

Choose a reason for hiding this comment

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

One thing to note: if this FHIR job fails midway, then you're only left with the indices that were able to be loaded. Is this the expected behavior that we want for upload?

Also, does this have performance detriments / is index generation relatively fast?

Copy link
Collaborator Author

@matthewpeterkort matthewpeterkort Sep 16, 2024

Choose a reason for hiding this comment

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

Not sure. Maybe create a Github issue on this repo and discuss with @bwalsh and others?

Choose a reason for hiding this comment

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

okay will write as a separate issue since an incomplete / failed job in previous versions still results in a borked ES index. See #40

Copy link

@quinnwai quinnwai left a comment

Choose a reason for hiding this comment

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

comments addressed

@matthewpeterkort matthewpeterkort merged commit 77756ee into development Sep 17, 2024
1 check passed
@matthewpeterkort matthewpeterkort deleted the feature/grip branch September 17, 2024 23:30
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