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

FI-3440: Add IG entity and repository, integrated into Evaluate task #573

Merged
merged 7 commits into from
Dec 19, 2024

Conversation

dehall
Copy link
Contributor

@dehall dehall commented Dec 12, 2024

Summary

This PR changes the following

  • Adds an IG entity class (and corresponding repository) that parses a TGZ file or directory and stores important components from it. This was partly based on existing code in test kits such as https://github.com/inferno-framework/us-core-test-kit/blob/main/lib/us_core_test_kit/generator/ig_loader.rb
  • Updates the Evaluate task to read an IG from a file on disk, or by identifier (for instance "[email protected]"). The download logic is handled by the existing IgDownloader, but the Evaluate task had to be modified slightly so that it would work
  • Updates the IgDownloader to take a path to write its file to. This allows it to write a downloaded IG to a temporary file instead of always igs/package.tgz

Some notes:

  • The IG class doesn't have a field for everything that could potentially be in an IG. Is there anything else we should include now for "future-proofing"?
  • I expect at some point we'll have a way to link a test kit or suite to an IG, but I didn't touch that idea here.
  • The evaluate task, if run multiple times with the same IG identifier that's not already cached, will download the package.tgz from the package server every time. Should we instead save it to a persistent location? Or save that for if/when this IG class is more integrated with the rest of inferno? I added support for reading from the user package cache at ~/.fhir/packages but I'm hesitant to write to it
  • Writing the downloaded IG to a temp file includes an ugly line in the output that I don't love but I'm not sure how to prevent this
  • This doesn't do anything when loading a file from the TGZ errors out, inspired by https://github.com/inferno-framework/us-core-test-kit/blob/main/lib/us_core_test_kit/generator/ig_loader.rb#L46 . I'm not sure how common errors here will be in practice given that we skip loading a lot of files, so I'm also open to just re-raising an error if one happens
  • The test case for reading an IG from a directory untars the tgz fixture to a temp dir. Originally I thought a shared/semi-persistent temp dir would be good, but that's tough to debug if things go wrong, and from experimentation the untar doesn't add noticeable time anyway (tests run in ~5.2s either way), so it's now untarred as needed. I expect we'll re-use the untarred version for some of the evaluator Rules unit tests.

Testing Guidance

Since the evaluator loads the IG but doesn't do anything useful yet, I suggest adding a little debug note like the following at the bottom of inferno-core/lib/inferno/apps/cli/evaluate.rb

        puts "Loaded #{_ig.id}: " \
           "#{_ig.profiles.length} profiles, " \
           "#{_ig.extensions.length} extensions, " \
           "#{_ig.value_sets.length} value sets, " \
           "#{_ig.examples.length} examples from #{ig_path} \n\n"

and running some examples in the evaluator task such as the following (which should all produce identical results):

bundle exec inferno evaluate [email protected]

bundle exec inferno evaluate ./spec/fixtures/uscore311.tgz

(the last two will only work if you already have the package cached via another utility such as the validator cli)
bundle exec inferno evaluate "hl7.fhir.us.core#3.1.1"

bundle exec inferno evaluate "~/.fhir/packages/hl7.fhir.us.core\#3.1.1"

Base automatically changed from FI-3376-evaluator-firststep to main December 12, 2024 16:39
@dehall dehall force-pushed the fi-3440-evaluator-ig-class branch from 3269b0d to eb0fa6d Compare December 12, 2024 16:47
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 93.82716% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.29%. Comparing base (c5ec6ae) to head (252796f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/inferno/entities/ig.rb 93.15% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #573      +/-   ##
==========================================
+ Coverage   84.22%   84.29%   +0.06%     
==========================================
  Files         272      274       +2     
  Lines       11584    11663      +79     
  Branches     1279     1302      +23     
==========================================
+ Hits         9757     9831      +74     
- Misses       1817     1822       +5     
  Partials       10       10              
Flag Coverage Δ
backend 92.61% <93.82%> (+0.01%) ⬆️
frontend 79.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Shaumik-Ashraf Shaumik-Ashraf left a comment

Choose a reason for hiding this comment

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

Tests all passed, and Thor CLI integration looks good.

Copy link
Collaborator

@Jammjammjamm Jammjammjamm left a comment

Choose a reason for hiding this comment

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

This doesn't seem like it will really be useable outside of the CLI unless IGs are stored in a repository.

An IG seems like an entity, so I think it should live in lib/inferno/entities and have an id. Then you can also add a basic in memory repository to go along with it. It doesn't have to be done as part of this PR, but I think ideally this would also check for IGs in ~/.fhir to avoid repeatedly downloading them.

@dehall dehall force-pushed the fi-3440-evaluator-ig-class branch 2 times, most recently from f150cd0 to 9ad449e Compare December 17, 2024 17:45
@dehall dehall changed the title FI-3440: Add IG class to DSL for Evaluator FI-3440: Add IG entity and repository, integrated into Evaluate task Dec 18, 2024
@dehall dehall force-pushed the fi-3440-evaluator-ig-class branch from e345225 to 5b469a5 Compare December 18, 2024 14:26
@@ -25,12 +25,12 @@ def load_ig(ig_input, idx = nil, thor_config = { verbose: true })
else
raise StandardError, <<~FAILED_TO_LOAD
Could not find implementation guide: #{ig_input}
Put its package.tgz file directly in #{ig_path}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note I deleted this line because it makes things a lot more complicated when using this with the evaluate rake task. Since we're just downloading to a temp folder in that case, I don't want to say "Put its package.tgz in /var/folders/l1/d3cs21v54hb4djpb1yjnlzm40000gn/T/inferno-core/" or whatever a temp folder path looks like, because doing that and trying the same thing wouldn't work. I'm open to wordsmithing and adding more params if we want to keep a note like this in the error message

@dehall dehall requested a review from Jammjammjamm December 18, 2024 14:41
Zlib::GzipReader.open(ig_path)
)

ig = IG.new({})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if you make the constructor take **params instead of just *params you should be able to do a plain IG.new without passing an empty hash.


file_name = relative_path.split('/').last

# TODO: consider making these regexes we can iterate over in a single loop
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need a TODO here.

@dehall dehall force-pushed the fi-3440-evaluator-ig-class branch from 91f2fad to 252796f Compare December 18, 2024 17:23
@dehall dehall requested a review from Jammjammjamm December 19, 2024 15:45
@dehall dehall merged commit 89c7b38 into main Dec 19, 2024
10 checks passed
@dehall dehall deleted the fi-3440-evaluator-ig-class branch December 19, 2024 17:41
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.

4 participants