Testing external contributor PR #313 by PlanXCyborg · Pull Request #315 · uc-cdis/gen3sdk-python

PlanXCyborg · 2026-08-24T14:43:28Z

*** DO NOT MERGE ***
This PR was created automatically to test an external PR
Tested PR link here

…ta files

… remove unecessary dependencies

github-actions · 2026-08-24T14:44:09Z

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

github-actions · 2026-08-24T15:13:21Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_dbgap.py	4	0	1	5
tests/test_ras_passport.py	0	0	2	2
tests/test_data_upload.py	8	0	1	9
tests/test_graph_submit_and_query.py	12	1	1	14
tests/test_presigned_url.py	8	0	0	8
tests/test_centralized_auth.py	5	0	0	5
tests/test_audit_service.py	1	0	0	1
tests/test_google_data_access.py	1	0	0	1
tests/test_gen3_sdk.py	1	0	0	1
TOTAL	40	1	5	46

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

Avantol13 · 2026-08-24T15:08:16Z

+        if force:
+            shutil.rmtree(output_dir, ignore_errors=True)
+
+    except BaseException as e:


You can't use BaseException here, it will completely absorb a CTRL+C from the terminal (KeyboardInterrupt). Ideally you shouldn't wrap so much code in a broad exception block. I would break the code out into a helper function so it's a bit cleaner to read, and while we don't encourage broad exception catching, if you do it, you should use the Exception class instead of BaseException.

also this needs to exit(1) after all the logging. This currently shows a "success" exit message

In the case that someone hits CTRL+C at the start of the run, do we want to treat it as a force delete and delete the posisbly corrupt files immadiately?

The reason for the long try/except block and the broad exception is to be able to remove files if --force is passed at any point in the run for whatever the reason it is that the run failed

no, ctrl+c is a kill immediately request from the user, so we should avoid capturing and doing anything else - consequences are the user's. So if they do that, then they may have to run force the next time for a clean run.

The broad exception is maybe okay, I'd just break it out to a helper function

Avantol13 · 2026-08-24T15:11:04Z

+        os.remove(f)
+
+
+def fhir_tagger(


we should rename this function to describe better the verb / action it takes. Currently it sounds like a class name (a thing that does something). I would argue for something like tag_fhir_resources_with_authz

Avantol13 · 2026-08-24T15:13:07Z

+        # initialize tagger
+        tagger = Gen3FHIRAuthzTagger(config_path=config)
+        # keep only relevant rules for the resource type
+        tagger.relevant_authz_rules(os.path.basename(input_file).split(".")[0])


This means that we rely explicitly on the filename for the FHIR resource type instead of checking the actual resourceType in the resource itself... I think we should reconsider this and instead read the first few lines of the file (and maybe last few) - you'll need this to be done without reading the whole file for efficiency, then check for that resourceType of those lines and if all the same: use that. and then make sure it's clear in docs that we expect 1 resource type per file

Done, but we can also check the resource type per record in transform chunk if necessary since we are already loading it

Avantol13 · 2026-08-24T15:14:17Z

+            if hook_result:
+                return hook_result
+
+        # check global catch-all fallback


it's not really a fallback b/c the rules never evaluate. it's an override

Avantol13 · 2026-08-24T15:15:36Z

+        return 0
+
+    # force to delete everything disregarding status
+    if force:


we should maybe only do this if --dry-run was not passed? I don't think it make sense to delete everything if it's a dry run - but if there's an argument for it, I'm open to it

The force option is for someone to pass in a normal run if they don't want any intermediates remaining. Otherwise, all intermediates remain in the temp folder. The dry-run option is in cleanup, where the user can choose whether they just want a list of files that would be deleted if they were to run the cleanup. Let me know if you want the dry-run as an option for the main pipeline as well.

nope it's okay to leave it out

Avantol13 · 2026-08-24T15:36:30Z

+            self.config = yaml.safe_load(f)
+        self.custom_hook = custom_hook
+
+    def relevant_authz_rules(self, resource_type: str):


We can make this even faster b/c fhirpathpy allows you to "compile" rules once and apply them over and over (which saves a good chunk of time). https://github.com/beda-software/fhirpath-py#compile

evaluate(resource, "Patient.gender = 'male'") takes the expression as a string and re-parses it every call. compile() does the parse once and hands back a callable that just applies the pre-parsed rule. Right now we reparse on every record for every rule.

You could build the callables once in relevant_authz_rules (where the rules are already being filtered) and call them in determine_authz instead of evaluate(resource, rule["condition"]).

Avantol13 · 2026-08-24T15:41:36Z

+
+@click.command(
+    context_settings={"help_option_names": ["-h", "--help"]},
+    help="Tag Bulk FHIR data with Gen3 compatible authorization tags",


You should add more detail here about the input_file, what type - what should be in it, etc
samre for output_file and config. The user needs all the detail about what to provide when they run poetry run gen3 fhir transform --help

Avantol13 · 2026-08-24T15:49:40Z

+    make_folders_for_filename(TMP_ROOT)
+
+    # make temp directories
+    hash = get_md5hash(input_file)


we may want to think through how to check _is_done before hashing where possible. B/c if this input is like 100GB, this is going to take a while even if it's already been run previuosly. Maybe that's okay

Avantol13 · 2026-08-24T15:53:26Z

+        raise click.UsageError("input_file and output_file must be different")
+
+    # only necessary the first time its run
+    make_folders_for_filename(TMP_ROOT)


don't do this b/c I think this treats the final /tmp as a filename.

just do: TMP_ROOT.mkdir(parents=True, exist_ok=True)

Avantol13 · 2026-08-24T16:19:24Z

+from collections.abc import Callable
+
+logging = get_logger(__name__)
+TMP_ROOT = pathlib.Path(".fhir_transform/tmp")


We shouldn't define this twice (here and CLI). ALso note that this is going to be scoped to the current directory
So resume only works if you re-run from the same directory, every directory you ever run from accumulates state, and cleanup only really sees the current directory.

Let's change it to a static location and allow it to be configured by an env var or passed argument (--work-dir). And maybe make a helper to get the right working dir.

DEFAULT_WORK_DIR = "~/.cache/gen3/fhir_transform" def resolve_work_dir(work_dir: str | os.PathLike[str] | None = None) -> pathlib.Path: root = pathlib.Path( work_dir or os.environ.get("GEN3_FHIR_WORK_DIR") or DEFAULT_WORK_DIR ).expanduser() root.mkdir(parents=True, exist_ok=True) # 0o700 makes it only only readable by owner and not everyone else (which is important # for potentially shared machines and potential FHIR PHI) root.chmod(0o700) return root

We'll need to update fhir cleanup (and everywhere else that was relying on this path) to go through this same resolver

Avantol13

see comments

…d functions, included more details in --help

github-actions · 2026-08-26T20:56:11Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_dbgap.py	4	0	1	5
tests/test_ras_passport.py	0	0	2	2
tests/test_data_upload.py	8	0	1	9
tests/test_graph_submit_and_query.py	12	1	1	14
tests/test_presigned_url.py	8	0	0	8
tests/test_centralized_auth.py	5	0	0	5
tests/test_audit_service.py	1	0	0	1
tests/test_google_data_access.py	1	0	0	1
tests/test_gen3_sdk.py	1	0	0	1
TOTAL	40	1	5	46

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

…h, update function descriptions

…t in the unit tests

github-actions · 2026-08-31T16:56:27Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_dbgap.py	4	0	1	5
tests/test_ras_passport.py	0	0	2	2
tests/test_data_upload.py	8	0	1	9
tests/test_graph_submit_and_query.py	12	1	1	14
tests/test_presigned_url.py	8	0	0	8
tests/test_centralized_auth.py	5	0	0	5
tests/test_audit_service.py	1	0	0	1
tests/test_google_data_access.py	1	0	0	1
tests/test_gen3_sdk.py	1	0	0	1
TOTAL	40	1	5	46

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

… through API instead

github-actions · 2026-08-31T20:38:18Z

Integration Tests

Test summary after running integration tests

filepath	passed	failed	skipped	SUBTOTAL
tests/test_dbgap.py	4	0	1	5
tests/test_ras_passport.py	0	0	2	2
tests/test_data_upload.py	8	0	1	9
tests/test_graph_submit_and_query.py	12	1	1	14
tests/test_presigned_url.py	8	0	0	8
tests/test_centralized_auth.py	5	0	0	5
tests/test_audit_service.py	1	0	0	1
tests/test_google_data_access.py	1	0	0	1
tests/test_gen3_sdk.py	1	0	0	1
TOTAL	40	1	5	46

Test summary after rerunning failed integration tests

filepath	passed	SUBTOTAL
tests/test_graph_submit_and_query.py	1	1
TOTAL	1	1

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

dsafarian and others added 16 commits August 14, 2026 08:54

Added cli/fhir_transform.py and transform_utils.py

fdc70f1

Add fhir_transform cli, utils, and tests

134df13

Remove unnecessary imports

92d603e

Add documentation, format tests, integrate into gen3 cli, add test_da…

44940a9

…ta files

Fix and add tests, create folder even if output_file path doesn't exist

aab4d54

Pass all tests, modify multiprocessing to not cause issues with gen3,…

c66e855

… remove unecessary dependencies

test md

404be39

Add fhir.md

8899bef

Merge branch 'uc-cdis:master' into fhir_transform

2d2e6fe

Black formatting

3f861f6

black formatting

80f1315

Fix pull request comments

d8ae042

Delete test outputs

76c6797

Remove setuptools dependency from pyproject.toml

a34ed6c

Fix pyproject.toml dependencies and poetry.lock

d929fd2

Remove FIXME

8e0174c

Avantol13 requested changes Aug 24, 2026

View reviewed changes

Fix default working directory and directory creation, imports, rename…

6192216

…d functions, included more details in --help

dsafarian added 8 commits August 27, 2026 10:44

Remove test outputs, fix typos, add assertions, change to SHA-256 has…

6b5f319

…h, update function descriptions

Fix hash bug

d00ecf5

Add unit test for global authz and fix tmp_path

8339418

Comment out all parrallelization code

9cbf8ba

Remove _resume_run from all code

8f5eee5

Fix exception handling

9cdf886

Fix resource_type check

818c537

Use fhirpathpy.compile instead of fhirpathpy.evaluate for speed up.

dd2abb2

dsafarian added 5 commits August 27, 2026 17:01

Add check for if output file exists and is not empty for _is_new.

157eb94

Fix poetry dependencies

d7773f6

Remove all parallelization code and make fhir import optional in cli

82e8710

Add helper function for tag_fhir_resources_with_authz and fix tmp_roo…

9c42dd7

…t in the unit tests

Black formatting

a9a0418

fix(drsclient): remove dep due to out of date sub-deps, implement DRS…

7b942b0

… through API instead

Merge branch 'master' into copy-of-external-pr-313

e7c6dac

Uh oh!

Conversation

PlanXCyborg commented Aug 24, 2026

Uh oh!

github-actions Bot commented Aug 24, 2026

Uh oh!

github-actions Bot commented Aug 24, 2026

Integration Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsafarian Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Avantol13 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 26, 2026

Integration Tests

Uh oh!

github-actions Bot commented Aug 31, 2026

Integration Tests

Uh oh!

github-actions Bot commented Aug 31, 2026

Integration Tests

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dsafarian Aug 27, 2026 •

edited

Loading