-
Notifications
You must be signed in to change notification settings - Fork 0
Update/versions #2
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
Changes from all commits
9d1debe
557d1ec
03bf0b8
9cae42c
4e214a2
0bb7985
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,7 +73,7 @@ async def _can_create(access_token: str, project_id: str) -> bool | str | int: | |
| return True, f"HAS SERVICE create on resource {required_service}", None | ||
|
|
||
|
|
||
| async def process(rows: List[dict], project_id: str, access_token: str) -> list[str]: | ||
| async def process(rows: List[dict], project_id: str, access_token: str) -> List[str] | None: | ||
| """Processes a bundle into a temp directory of NDJSON files | ||
| that are compatible with existing loading functions | ||
|
|
||
|
|
@@ -109,14 +109,31 @@ async def process(rows: List[dict], project_id: str, access_token: str) -> list[ | |
| temp_file.close() | ||
|
|
||
| if files_written: | ||
| subprocess.run(["jsonschemagraph", "gen-dir", "iceberg/schemas/graph", f"{temp_dir}", f"{temp_dir}/OUT", "--project_id", f"{project_id}", "--gzip_files"]) | ||
| res = bulk_load(await _get_grip_service_name(), await _get_grip_graph_name(), project_id, f"{temp_dir}/OUT", logs, access_token) | ||
| if int(res[0]["status"]) != 200: | ||
| server_errors.append(res[0]["message"]) | ||
| program, project = project_id.split("-") | ||
| project_str_dict = f'{{"auth_resource_path":"/programs/{program}/projects/{project}"}}' | ||
| print(f"Using project: {project_str_dict}") | ||
| result = subprocess.run( | ||
| ["jsonschemagraph", "gen-dir", "iceberg/schemas/graph", f"{temp_dir}", f"{temp_dir}/OUT", "--extraArgs", project_str_dict, "--gzip_files"], | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wanted to build jsonschemagraph from
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that's because grip was using a shadow branch. See: bmeg/jsonschemagraph#16 try with which is referencing the version from bmeg/jsonschemagraph@b88905d But for this PR I believe it's using a executable from this release https://github.com/bmeg/jsonschemagraph/releases/tag/v0.0.3 if you check the Dockerfile There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay so to confirm: all dockerfile dependencies point to merged branches for iceberg and jsonschema?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. forgot about this repo. Had to make a new PR now that you mentioned it. bmeg/jsonschemagraph#16. Since it's working, if you want to review it you can, but not going to make any changes to it that impact this release since it's working. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eems like there's at least one other dependency Since it's statically compiled in Go, I'm less worried about updates to the PR causing breaking changes but I want us to be more stringent in future releases: all dependencies should point to merged code if it's a feature in the release. I'm good to proceed if we start enforcing this rule in future releases. How do I test these changes?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. grip is using this jsonschemagraph directly as a package. if jsonschemagraph doesn't work you will know it in downstream code repositories, and ultimately in the g3t end to end test Kyle had approved this PR in grip, that requires this dependency. You test these changes using the g3t end to end test like what is specified int he release notes |
||
| capture_output=True, | ||
| text=True, | ||
| check=False | ||
| ) | ||
| if result.returncode == 0: | ||
| print("jsonschemagraph ran successfully.") | ||
| res = bulk_load(await _get_grip_service_name(), await _get_grip_graph_name(), project_id, f"{temp_dir}/OUT", logs, access_token) | ||
| if int(res[0]["status"]) != 200: | ||
| server_errors.append(res[0]["message"]) | ||
| else: | ||
| print(f"jsonschemagraph failed with exit code: {result.returncode}") | ||
| print("Stdout:") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just confirming, the stdout gets logged to the FHIR server pod logs?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any error logs will come as a https response message. Any more in depth logs can be checked by checking the fhir server logs in k8s. |
||
| print(result.stdout) | ||
| print("Stderr:") | ||
| print(result.stderr) | ||
| server_errors.append(f"jsonschemagraph failed: {result.stderr.strip()}") | ||
|
|
||
| try: | ||
| db = LocalFHIRDatabase(db_name=f"{temp_dir}/local_fhir.db") | ||
| db.bulk_insert_data(resources=get_project_data(await _get_grip_service_name(), await _get_grip_graph_name(), project_id, logs, access_token)) | ||
| db.bulk_insert_data(resources=get_project_data(await _get_grip_service_name(), await _get_grip_graph_name(), project_id, logs, access_token, 1024*1024)) | ||
|
|
||
| index_generator_dict = { | ||
| 'researchsubject': db.flattened_research_subjects, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just main or the most recent commit in iceberg? If there's a tendency to not do main then we should have a development branch or something in iceberg to pin it to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because the server is setup to use a very specific version of iceberg. See
gen3-helm/helm/grip/templates/post-something.yamlI want to match what I know the server is using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok