Integrate FHIR Package Loader v2 #1543

cmoesel · 2024-12-06T17:42:42Z

Description: This PR integrates the brand new FHIR Package Loader (v2). The general approach was:

Update FHIRDefinitions to extend BasePackageLoader
Update fishing functions in FHIRDefinitions to use the findResource and findResourceInfo functions in BasePackageLoader -- ensuring that FHIRDefinitions still conforms to the expected Fisher interface.
Remove most other functions from FHIRDefinitions in favor of the functions inherited from BasePackageLoader
Update all affected code as necessary (most significant changes in package loading and IG Exporter)
Update tests to load most fixtures using FPL virtual packages

This also updates the regression framework to support pre-caching of dependencies so the first run of SUSHI does not have the extra work of downloading all dependencies (which could skew results). It also reports out more information regarding performance.

Testing Instructions: Some ideas:

Run it against some of your favorite projects
Delete or rename your FHIR cache and try it on a project or two
Try package-related features like sushi update-dependencies
Try the FSH-to-FHIR API
Run a regression.
- Note that the regression is expected to result in changes for a few repos (mainly some differences in warning/error counts). Confirm that the changes appear to be correct and/or harmless.

First take at integrating the new FPL. It works, but could use some refactoring, cleanup, and optimization.

Additional changes to get all of the tests passing again. Still needs refactoring, cleanup, and optimization.

Clean up the FHIRDefinitions objects to only contain what's actually needed.

…reporting

- Only clone FPL results where and when necessary - Use FPL's new SafeMode.FREEZE feature to avoid unintentional modification of cached resources - Integrate optimized FPL with new optimize() function - Update some outdated dependencies

jafeltra

I just left a few very small questions/comments.

I tried out using this branch, running a small regression, using the update-dependencies command, and the FSH to FHIR API, and everything worked as expected!

package.json

src/app.ts

src/fhirdefs/FHIRDefinitions.ts

src/ig/IGExporter.ts

mint-thompson

This worked perfectly when I ran with it locally. I have a few small comments and suggestions.

src/app.ts

mint-thompson · 2024-12-10T14:45:46Z

src/ig/IGExporter.ts

@@ -271,15 +273,16 @@ export class IGExporter {
    }

    if (dependsOn.version === 'latest') {
+      // TODO: This assumes only a single version of a package is in scope


The current implementation is fine, since it's the same as before, but I'm curious: what would be the best way to handle latest when multiple versions are in scope? Would it just be whichever version is the latest based on a semver comparison? Or would we need to check the registry to see what has the tag?

I think we have a few options. Using semver would probably be a reasonable approach, but we might also look into using FPL to find the latest again or tagging latest versions in the FPL DB when we put them in there (and exposing that in the PackageInfo results.

mint-thompson · 2024-12-10T18:14:45Z

src/ig/predefinedResources.ts

+  );
+  if (configParameters && projectDir) {
+    const pathResources = configParameters
+      ?.filter(parameter => parameter.value && parameter.code === 'path-resource')


The if statement guarantees that configParameters is truthy, so I don't think you need to null-coalesce here.

Suggested change

?.filter(parameter => parameter.value && parameter.code === 'path-resource')

.filter(parameter => parameter.value && parameter.code === 'path-resource')

Addressed in 5b4874e.

mint-thompson · 2024-12-11T14:24:00Z

test/fhirtypes/ElementDefinition.assignFshReference.test.ts

-        );
+      beforeAll(async () => {
+        r5Defs = await getTestFHIRDefinitions(false, testDefsPath('r5-definitions'));
+        r5Fisher = new TestFisher().withFHIR(r5Defs);


Is this line needed? r5Fisher gets assigned to a new TestFisher during beforeEach, and nothing uses it before then.

Addressed in 5b4874e.

mint-thompson · 2024-12-11T14:25:09Z

test/fhirtypes/ElementDefinition.assignValue.test.ts

-        );
+      beforeAll(async () => {
+        r5Defs = await getTestFHIRDefinitions(false, testDefsPath('r5-definitions'));
+        r5Fisher = new TestFisher().withFHIR(r5Defs);


Similarly to ElementDefinition.assignFshReference.test.ts, I don't think this line should be needed. This also appears to be in a lot of the ElementDefinition.assign{something} tests. Maybe I've missed something about side effects?

No, I think this is just the result of me having to refactor these tests and doing some of the refactoring a line at a time without looking at the overall test suite to notice that some things are no longer necessary. Good find.

This also appears to be in a lot of the ElementDefinition.assign{something} tests. Maybe I've missed something about side effects?

Actually, now that I've looked through them, I don't see it anywhere else. Just ElementDefinition.assignFshReference.test.ts and ElementDefinition.assignValue.test.ts. Are you sure you saw it in other tests? And if so, where?

I must have been looking at the wrong set of search results when I wrote this comment if I thought there were many other places. I only see unnecessary instantiation of TestFisher in a beforeAll block in those two files.

Addressed in 5b4874e.

mint-thompson · 2024-12-11T14:29:08Z

test/fhirtypes/ElementDefinition.constrainType.test.ts

-      );
-      defs.add(def);
+    it('should allow a choice to be constrained to a profile of Reference', async () => {
+      defs.loadLocalPaths(testDefsPath('StructureDefinition-reference-with-type.json'));


Should this line have an await to guarantee that the local definition finishes loading before trying to constrain the element's type?

Addressed in 5b4874e -- and I found one more case of loadLocalPaths that needed an await.

mint-thompson · 2024-12-11T15:23:31Z

test/utils/Processing.test.ts

 const NUM_R5_AUTO_DEPENDENCIES = 3;

-const TERM_R4_PKG_RESPONSE = {


I am a big fan of how much simpler these test definitions get to be now.

The main difference in FPL 2.0.0-beta.2 is that you no longer pass in a sql.js Database to the SQLJSPackageDB constructor. Instead, you construct SQLJSPackageDB and then call its async initialize function. This required us to update FHIRDefinitions to add an initialize function that then invokes the SQLJSPackageDB initiailze function.

mint-thompson

This looks good! I have one minor comment, but I'm fine with this being merged in as-is.

mint-thompson · 2024-12-16T19:01:16Z

src/fhirdefs/FHIRDefinitions.ts

+export async function newFHIRDefinitions(
+  isSupplementalFHIRDefinitions = false,
+  supplementalFHIRDefinitionsFactory?: () => Promise<FHIRDefinitions>,
+  // override is mainly intended to be used in unit tests
+  override?: {
+    packageDB?: PackageDB;
+    packageCache?: PackageCache;
+    registryClient?: RegistryClient;
+    currentBuildClient?: CurrentBuildClient;
+    options?: BasePackageLoaderOptions;
+  }
+) {
+  const fhirDefinitions = new FHIRDefinitions(
+    isSupplementalFHIRDefinitions,
+    supplementalFHIRDefinitionsFactory,
+    override
+  );
+  await fhirDefinitions.initialize();
+  return fhirDefinitions;
+}
+


This implementation is good, but I'm wondering if I like this better as a static method on FHIRDefinitions instead of a standalone function. Also, the function name seems risky for awkward typos, since this and the constructor take the same arguments.

cmoesel · 2024-12-17T18:20:36Z

@jafeltra @mint-thompson - I pushed up some commits that change newFHIRDefinitions to createFHIRDefinitions and integrate FPL 2.0.0-beta.3.

jafeltra

Yay this looks good to me! All the fixes look good since my last review!

mint-thompson

hooray!

cmoesel · 2024-12-20T16:09:50Z

Excellent. This signals that the new FPL is also ready for official release. So here is the plan:

Bump the FPL version to 2.0.0 (non-beta) and release on NPM.
Integrate the official FPL 2.0.0 release into this PR.
Since FPL 2.0.0 is the same as FPL 2.0.0-beta.3, no further review of this PR is required after that, so I will merge this one.
Release this as a minor SUSHI release.

cmoesel · 2024-12-20T17:33:33Z

@jafeltra and @mint-thompson - I dismissed your reviews when I pushed up the new dependency on FPL 2.0.0. As noted above, it does not affect any actual logic since FPL 2.0.0 is exactly the same as FPL 2.0.0-beta.3. Please re-approve.

cmoesel added 8 commits December 6, 2024 12:46

New FPL Take 1

332a170

First take at integrating the new FPL. It works, but could use some refactoring, cleanup, and optimization.

New FPL Take 2

6695424

Additional changes to get all of the tests passing again. Still needs refactoring, cleanup, and optimization.

New FPL Take 3

204461a

Clean up the FHIRDefinitions objects to only contain what's actually needed.

Regression improvements: summary, dependency precaching, performance …

ac0d4f2

…reporting

FHIR Package Loader Optimization

648bbd6

- Only clone FPL results where and when necessary - Use FPL's new SafeMode.FREEZE feature to avoid unintentional modification of cached resources - Integrate optimized FPL with new optimize() function - Update some outdated dependencies

Use [email protected] from NPM

3911e2c

Remove mistakenly committed FPL.sqlite file

002767b

Fix test broken by rebase

4512690

cmoesel force-pushed the new-fpl branch from f359a33 to 4512690 Compare December 6, 2024 17:54

cmoesel marked this pull request as ready for review December 6, 2024 17:55

jafeltra reviewed Dec 9, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/app.ts Outdated Show resolved Hide resolved

src/app.ts Outdated Show resolved Hide resolved

src/fhirdefs/FHIRDefinitions.ts Outdated Show resolved Hide resolved

src/ig/IGExporter.ts Show resolved Hide resolved

mint-thompson requested changes Dec 11, 2024

View reviewed changes

Minor updates based on PR review

5b4874e

cmoesel mentioned this pull request Dec 12, 2024

Refactor sql.js out of SQLJSPackageDB constructor FHIR/fhir-package-loader#45

Merged

mint-thompson previously approved these changes Dec 17, 2024

View reviewed changes

cmoesel added 2 commits December 17, 2024 11:24

Rename newFHIRDefinitions to createFHIRDefinitions

d51318c

Integrate FPL 2.0.0-beta.3

7761842

cmoesel dismissed mint-thompson’s stale review via 7761842 December 17, 2024 17:37

cmoesel and others added 2 commits December 18, 2024 09:56

Merge branch 'master' into new-fpl

9ba90bb

Fix broken test after merge

add3588

jafeltra previously approved these changes Dec 20, 2024

View reviewed changes

mint-thompson previously approved these changes Dec 20, 2024

View reviewed changes

Integrate FPL 2.0.0

f9a7d8d

cmoesel dismissed stale reviews from mint-thompson and jafeltra via f9a7d8d December 20, 2024 17:30

jafeltra approved these changes Dec 20, 2024

View reviewed changes

mint-thompson approved these changes Dec 20, 2024

View reviewed changes

cmoesel merged commit 6541681 into master Dec 20, 2024
14 checks passed

cmoesel deleted the new-fpl branch December 20, 2024 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate FHIR Package Loader v2 #1543

Integrate FHIR Package Loader v2 #1543

cmoesel commented Dec 6, 2024 •

edited

Loading

jafeltra left a comment

mint-thompson left a comment

mint-thompson Dec 10, 2024

cmoesel Dec 12, 2024

mint-thompson Dec 10, 2024

cmoesel Dec 12, 2024

mint-thompson Dec 11, 2024

cmoesel Dec 12, 2024

mint-thompson Dec 11, 2024

cmoesel Dec 12, 2024

cmoesel Dec 12, 2024

mint-thompson Dec 12, 2024

cmoesel Dec 12, 2024

mint-thompson Dec 11, 2024

cmoesel Dec 12, 2024

mint-thompson Dec 11, 2024

cmoesel Dec 12, 2024

mint-thompson left a comment

mint-thompson Dec 16, 2024

cmoesel commented Dec 17, 2024

jafeltra left a comment

mint-thompson left a comment

cmoesel commented Dec 20, 2024

cmoesel commented Dec 20, 2024

	?.filter(parameter => parameter.value && parameter.code === 'path-resource')
	.filter(parameter => parameter.value && parameter.code === 'path-resource')

		const NUM_R5_AUTO_DEPENDENCIES = 3;

		const TERM_R4_PKG_RESPONSE = {

Integrate FHIR Package Loader v2 #1543

Integrate FHIR Package Loader v2 #1543

Conversation

cmoesel commented Dec 6, 2024 • edited Loading

jafeltra left a comment

Choose a reason for hiding this comment

mint-thompson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mint-thompson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmoesel commented Dec 17, 2024

jafeltra left a comment

Choose a reason for hiding this comment

mint-thompson left a comment

Choose a reason for hiding this comment

cmoesel commented Dec 20, 2024

cmoesel commented Dec 20, 2024

cmoesel commented Dec 6, 2024 •

edited

Loading