FHIR: Output NPM format

[joeflack4]

Updates

Details

FHIR: Output as NPM package

• Rename: OboGraphToFHIRConverter --> OboGraphToFhirJsonConverter
• Add: OboGraphToFhirNpmConverter: Saves in FHIR NPM package format.
• Add: New CLI output_type option: fhirnpm
• Add: StreamingFhirNpmWriter (WIP)
• Add: Test file: tests/input/fhir_npm_manifest_so.json
• Add: Test helper function: _load_and_convert_npm()
• Add: Unit test: test_convert_so_package()
• Update: .gitignore: tests/input/*_conf.json

FHIR: Output ConceptMap JSONs

• Update: OboGraphToFhirJsonConverter: Now also saves ConceptMaps

[codecov-commenter]

Codecov Report

Patch coverage: 58.97% and project coverage change: -0.01 ⚠️

Comparison is base (32556dc) 78.58% compared to head (00e0e15) 78.57%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #511      +/-   ##
==========================================
- Coverage   78.58%   78.57%   -0.01%     
==========================================
  Files         216      216              
  Lines       24622    24653      +31     
==========================================
+ Hits        19349    19371      +22     
- Misses       5273     5282       +9     

Impacted Files	Coverage Δ
src/oaklib/io/streaming_fhir_writer.py	41.66% <33.33%> (-8.34%)	⬇️
...c/oaklib/converters/obo_graph_to_fhir_converter.py	88.50% <40.00%> (-6.37%)	⬇️
src/oaklib/cli.py	60.46% <100.00%> (+0.01%)	⬆️
src/oaklib/interfaces/dumper_interface.py	88.88% <100.00%> (ø)
tests/test_converters/test_obo_graph_to_fhir.py	61.40% <100.00%> (+9.02%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[joeflack4]

Outdated changes summary 4/5/2023

@cmungall There is only 1 issue that is keeping the linter unhappy, which I tagged Harshad about. It's related to my unit test unzipping my archive. Other than that, this is ready for your review.

Note that merging this is not urgent. But would be nice to have done within ~2-3 weeks.

[joeflack4]

StreamingFhirNpmWriter (@cmungall question)

Details

@cmungall I didn't even notice these "streaming writers" existed until a short time ago. Do you need me to implement StreamingFhirNpmWriter? Does it make sense to?

edit: If so, I don't have a lot of time so it would be better for me to do in multiple PRs and track progress in an issue.

[joeflack4]

StreamingFhirJsonWriter .dumps() bug? @cmungall

Details

This isn't code I wrote. I'm on not even sure how StreamingFhirJsonWriter is used, but I noticed it dumps like this:
self.file.write(json_dumper.dumps(code_system))

But I would expect it to do something like this:
OboGraphToFhirJsonConverter.dump(...)

Should I create an issue?
I could maybe try to evaluate it as well.

[joeflack4]

OboGraphToFhirNpmConverter structure and pattern differences (@cmungall review)

Details

@cmungall Basically the review I want here is that the nature of the FHIR output is now starting to deviate from what is standard in OAK. It's not just 1 file in, 1 file out for this. FHIR will now output a number of files, and if they're using the fhirnpm format, it'll be 1 zip.

---

@cmungall I followed your suggestion and made a fhirnpm output_type. Given the architecture of OAK, I think a OboGraphToFhirNpmConverter is expected. And it seemed to make sense to subclass it from OboGraphToFhirJsonConverter.

Non-standard dump() and alternative idea

All it needs to do is override dump() and do some extra things. But this might violate your intentions of standardization around the dump() methods in these classes. Is it OK how I've done this? Or do you want me to create some kind of intermediate method, say, package() which comes after the inherited OboGraphToFhirJsonConverter.convert() and then returns back to dump()? If so, perhaps package() could do everything up to creating the actual zip file.

Required dump() params

Some other non-standard changes that I've made to OboGraphToFhirNpmConverter.dump() given its special case:

• target is now a required param, and in this case represents the output directory. I could change this to be the output path of the zip file. However, it seems that it must be required, because the other methods print the output to the terminal, and I imagine we don't want to do that with a zip file.
• manifest_path a required parameter. If you like my idea of creating a package() method, I could pass this down and require it there instead.
• kwargs["code_system_id"]: I didn't make this a required param (yet), but it is technically required.
• kwargs["code_system_url"]: I didn't make this a required param (yet), but it is technically required.

PyCharm signature method warning

I have a warning:

Signature of method 'OboGraphToFhirNpmConverter.dump()' does not match signature of the base method in class 'OboGraphToFhirJsonConverter'

It's unhappy that I made target a required param by removing = None. But I don't see why this is invalid / bad. The code seems to run fine.

I can't seem to put a JetBrains noinspection comment to ignore the warning, either.

[joeflack4]

fhirjson_conf.json setup (@cmungall review)

@cmungall Harder to review as time goes on as fades in back of memory. TLDR this is a minor choice; I think its (a) dynamically generate .gitignored conf JSONs , or (b) commit them as static files in test/input/. Existing / current pattern is(a).

Details 2023/11/26

We were also discussing in this comment. I set it up to write to a file. I think I followed an existing pattern you were using. I think I would prefer just statically saving as JSON and loading that way.

ontology-access-kit/tests/test_cli.py

Lines 503 to 507 in b42d286

	fhir_conf = {
	"code_system_id": "test",
	"code_system_url": "http://purl.obolibrary.org/obo/go.owl",
	"native_uri_stems": ["http://purl.obolibrary.org/obo/GO_"],
	}

ontology-access-kit/tests/test_cli.py

Line 525 in b42d286

output_path = str(OUTPUT_DIR / f"test_dump-{output_format}.out")

Details 2023/04/05

@hrshdhgd I should have made a 'review comment' earlier when tagging you. I like being able to hit 'resolve' to clean the page up a bit.

Regarding fhirjson_conf.json, I located the source. It's from a (very helpful) unit test that @cmungall added in another PR recently. Basically, in order to keep the CLI clean, we're planning on having some dumpers, etc, pull their parameters from a config file rather than declaring them in cli.py in the normal click way. Right now, the only such dumper output_type that has such a config is fhirjson.

ontology-access-kit/tests/test_cli.py

Lines 477 to 479 in 22eebdf

	conf_path = INPUT_DIR / f"{output_format}_conf.json"
	with open(conf_path, "w", encoding="utf-8") as f:
	json.dump(conf_object, f)

@cmungall It does look like this probably should be in the .gitignore, so I've added the following entry (though let me know if you actually do want to have these committed):
tests/input/*_conf.json

[joeflack4]

CLI & documenting config @joeflack4

I think this may still need:

• 1. CLI
• 2. Docs about the FHIR config JSON

Details

Tasks

1. CLI

It can call the dumper for fhirnpm, but need to settle on the params for it first.

2. Docs about the FHIR config JSON

Should probably document the config for that as with the fhirjson dumper.

Discussion

Joe 4/5/2023:
I haven't actually properly set up these yet.

Joe 9/2023:
You may consider this a blocker, the fact that there's no CLI. Since I didn't do it back in April, I'm thinking it's because I wanted your input on the params for some reason. If this is needed for merge, I can put this back into draft mode, look into it and let you know if/where I need feedback.

Joe 11/26/2023:
I think I may have time to do this in this PR now.