-
Notifications
You must be signed in to change notification settings - Fork 5
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-3376 Migrate Evaluator CLI into inferno core CLI #557
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #557 +/- ##
==========================================
- Coverage 84.44% 84.17% -0.28%
==========================================
Files 265 271 +6
Lines 11488 11581 +93
Branches 1266 1277 +11
==========================================
+ Hits 9701 9748 +47
- Misses 1777 1823 +46
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 I think this is a good start for us. I suggest leaving a couple things out for now until we know they're needed. I also think for any nontrivial additions to core we need a review from @Jammjammjamm
lib/inferno/dsl/fhir_evaluator/ig.rb
Outdated
require 'rubygems/package' | ||
require 'zlib' | ||
|
||
module FhirEvaluator |
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.
I guess it doesn't hurt to leave this class in, but I thought migrating this over might be best left for the next step since we need to integrate with the validator, so the IG tgz will need to be in a specific folder. (Or we'll need to load from a package server)
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.
I wonder where this will be migrated, do you think it will be part of Inferno utils?
Does Inferno have similar util already (extracting key contents from IGs) that can reuse to extend?
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.
Good question. There's nothing in core that parses an IG that I'm aware of, but there are things in various test kits that this was inspired by, for example these in us-core-test-kit
https://github.com/inferno-framework/us-core-test-kit/blob/main/lib/us_core_test_kit/generator/ig_loader.rb
https://github.com/inferno-framework/us-core-test-kit/blob/main/lib/us_core_test_kit/generator/ig_resources.rb
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.
I believe https://oncprojectracking.healthit.gov/support/browse/FI-3440 concerns this issue, which is next phase of the integration. I am okay to do it in this PR though it looks like phase 1 is already overwhelmed by many issues.
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.
Yeah that's what I meant before - since we have a separate task for migrating this IG class and the class isn't used yet, it might be easier to just leave it out for now
Right, added him for review. |
lib/inferno/dsl/fhir_evaluation.rb
Outdated
end | ||
|
||
require_relative 'fhir_evaluator/cli' | ||
FhirEvaluator::CLI.start |
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.
The evaluator is expected to work both in and out of the CLI, so there should be no CLI-specific code inside inferno/dsl
.
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.
The evaluator CLI is removed.
require 'yaml' | ||
require 'pathname' | ||
|
||
module FhirEvaluator |
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.
I don't understand why this is separate from apps/cli/evaluator.
Everything in inferno needs to be inside the Inferno
module. Anything inf inferno/dsl should be inside Inferno::DSL
.
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.
The evaluator CLI is removed and the evaluator is called from apps/cli/evaluator.
end | ||
|
||
# Get the subclasses of this class, ie, the actual implemented Rules. | ||
def self.descendants |
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.
I think Class.descendants
already exists and you shouldn't have to reimplement it.
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.
Removed
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.
You don't need the inherited
hook either.
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.
Removed
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.
The US Core IG should not be included. I also expect to see some unit tests.
lib/inferno/dsl/fhir_evaluation.rb
Outdated
module FHIREvaluation | ||
class Evaluator | ||
def initialize(ig_path, data_path) | ||
Dotenv.load |
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.
This shouldn't have to deal with Dotenv
. Inferno itself should handle that.
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.
Removed
lib/inferno/dsl/fhir_evaluation.rb
Outdated
module Inferno | ||
module DSL | ||
module FHIREvaluation | ||
class Evaluator |
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.
This all seems like CLI-specific code that should belong in the CLI.
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.
Based on conversation with Jaehoon - this file will be merged into the apps/cli/evaluator.rb
file
module FhirEvaluator | ||
# DataSummary represents the results of performing data characterization. | ||
class DataSummary | ||
attr_accessor :root_resource_ids, # All Example (root resource) Ids |
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.
All the comments are still there.
:resource_patient_map, # Resources and corresponding Patient Ids as subject | ||
:resource_subject_map # Resources and corresponding subject | ||
|
||
def initialize(_data) |
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 does this constructor have an unused argument? Is there a reason it isn't being stored in an instance variable so that it doesn't have to be passed around to various methods?
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.
Per discussion with Dylan, we wll remove this file. The comment is archived at https://gitlab.mitre.org/inferno/fhir_evaluator/-/issues/1. We will come back to this when we decide to reuse.
end | ||
end | ||
|
||
def to_json(*_args) |
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.
A method called to_json
needs to return a json string. If you need a method which builds the hash which to_json
stringifies, you can call it as_json
.
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.
Per discussion with Dylan, we wll remove this file. The comment is archived at https://gitlab.mitre.org/inferno/fhir_evaluator/-/issues/1. We will come back to this when we decide to reuse.
lib/inferno/dsl/fhir_evaluator/ig.rb
Outdated
attr_accessor :profiles, :extensions, :value_sets, :search_params, :examples | ||
|
||
def initialize(ig_path) | ||
raise "#{ig_path} is not an IG file" unless File.file?(ig_path) |
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.
This constructor is doing way too much. It shouldn't be doing much more than assigning instance variables. Make a class method that initializes the object and then performs the operations rather than having it all done in the constructor.
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.
The file is deleted. IG import will be handled at phase 2.
|
||
module FhirEvaluator | ||
module Rules | ||
class HasExamples < Rule |
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.
This class name sounds like a module rather than a class. The filename needs to match the class/module name.
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.
The file is deleted.
@@ -0,0 +1,96 @@ | |||
module FhirEvaluator |
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.
Anything inside Inferno needs to be inside the Inferno
module. Anything in inferno/dsl
needs to be inside Inferno::DSL
.
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.
The file is deleted.
lib/inferno/dsl/fhir_evaluation.rb
Outdated
class Evaluator | ||
def initialize(ig_path, data_path) | ||
Dotenv.load | ||
Dir.glob(File.join(__dir__, 'fhir_evaluator', 'rules', '*.rb')).each do |file| |
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.
This should not be happening as part of the constructor. It should go with the other requires.
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.
Commented out this part for now as we don't read rules at initial integration, will come back at phase 3.
# @resource_subject_map = data.map { |e| resources_subjects(e) }.flatten.uniq | ||
end | ||
|
||
def extract_resources_ids(resource) |
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.
These extract_noun1_noun2
method names don't make sense to me. Is it extracting the first noun, the second noun, or both? They should be renamed.
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.
Per discussion with Dylan, we wll remove this file. The comment is archived at https://gitlab.mitre.org/inferno/fhir_evaluator/-/issues/1. We will come back to this when we decide to reuse.
lib/inferno/dsl/fhir_evaluation.rb
Outdated
module Inferno | ||
module DSL | ||
module FHIREvaluation | ||
class Evaluator |
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.
Based on conversation with Jaehoon - this file will be merged into the apps/cli/evaluator.rb
file
@@ -0,0 +1,80 @@ | |||
# Common configuration. |
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.
From discussion - this file should be as empty as possible for the initial impl
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.
Keep configurations only actively used.
Removed all Rule configurations for now as we don't run any rules for now.
@@ -0,0 +1,96 @@ | |||
module FhirEvaluator |
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.
From discussion - delete this file
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.
Deleted
# A Dataset returns an Array of FHIR data to be summarized or evaluated, | ||
# with convenience methods for loading from a file path or from an | ||
# Array of FHIR JSON strings. | ||
module DatasetLoader |
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.
From discussion - update filename to match module name
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.
Updated.
@@ -0,0 +1,81 @@ | |||
module Util |
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.
From discussion - delete this file but preserve Steve's comments in a GitLab issue for when we eventually bring this in
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.
Deleted
lib/inferno/dsl/fhir_evaluator/ig.rb
Outdated
@@ -0,0 +1,89 @@ | |||
# frozen_string_literal: true |
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.
From discussion - delete this file; it will be added back in FI-3440 as noted. I copied Steve's comment into that issue on Jira
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.
Deleted.
@@ -0,0 +1,27 @@ | |||
# frozen_string_literal: true |
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.
From discussion - delete this file, note Steve's comment
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.
Deleted
@@ -0,0 +1,44 @@ | |||
# frozen_string_literal: true |
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.
From discussion - delete this file
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.
Deleted
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.
From discussion - delete this file
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.
Deleted.
context.results | ||
end | ||
|
||
def validate_args(ig_path, data_path) |
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.
I think we had a miscommunication somewhere - everything related to file paths and reading files should be in the CLI, in apps/cli/evaluator.rb
. Apologies the file names are the same but I'm not sure what better unique names might be.
So pretty much everything in this file now except the evaluate method should be migrated over there. I think this file was good as of this earlier revision: https://github.com/inferno-framework/inferno-core/blob/5d1ef09fc3cfe428eb52ef0b5be0ec2f90e15f33/lib/inferno/dsl/fhir_evaluator/evaluator.rb
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.
I believe you mentioned CLI can include actual logics to run the evaluator, but I missed it. I will move them over under CLI.
Speaking of file names, is the subfolder name dsl/fhir_evaluator
good, or dsl/fhir_evaluation
is even better?
Looking around other DSLs, they are like fhir_validation
or fhirpath_evaluation
. But some are like fhir_client_builder
. Looks like some mix of ~on
and ~er
.
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.
Sure I'm fine with changing names to make them more consistent with the rest of the system. Since the validator is module FHIRResourceValidation
, class Validator
, we would probably want this module to be FHIREvaluation
and the DSL class to remain as Evaluator
. If it helps distinguish things, maybe we want the cli class to be apps/cli/evaluate.rb
instead of apps/cli/evalutator.rb
?
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.
Agreed. evaluate
is more aligned with others e.g. execute
.
|
||
raise(TypeError, 'Malformed configuration') unless @data.is_a?(Hash) | ||
|
||
# def method_missing(name, *args, &) |
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.
I assume the point of all this method_missing
stuff and Section class was so that nested fields could be referenced as config.rule.rule_name.whatever
instead of config['rule']['rule_name']['whatever']
. If we don't want to do that anymore we should delete this rather than comment it out. Or if we do want to support that, uncomment this. I feel like it's nice but not necessary
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.
I think it is good to have in the end, but we discussed config.yml
to have it simple at this phase and I removed all Rule related configurations. So for now we aren't using this as there's no configuration to call. I will remove this and come back to this if we have clear picture of config
later.
class Evaluator | ||
attr_accessor :data, :config | ||
|
||
def initialize(data, config) |
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.
I haven't kept up with all of @Jammjammjamm 's comments so I hope I'm not suggesting a reversion of one of them here, but the method signatures in this class are kind of reversed from how they originally were in the gitlab repo:
def initialize(the_ig)
@ig = the_ig
end
def evaluate(data, summary = nil, config = Config.new)
At minimum the ig param will need to be added back in somewhere.
But the way I originally envisioned this is that one could theoretically create one evaluator per IG, and then call evaluate on multiple sets of data. As it is now I guess I don't see the benefit of the constructor and the attr_accessors, everything is pretty contained in the evaluate method.
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.
I actually removed the IG part while refactoring evaluator
as you took over the IG task and expected to handle whatever about IG. I will put IG attr_accessors back in constructor.
If the evaluator is designed to use for one IG with multiple data, Yes, data shouldn't be attr_accessors. About Config, for now it is fixed and universal for all data, but we only have minimum (almost empty) there. I will put it under evaluate
as you suggested.
Co-authored-by: Dylan Hall <dehall@mitre.org>
@@ -0,0 +1,9 @@ | |||
Environment: |
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.
I definitely don't think we should have a folder just for this one file. Is there a bunch of stuff that will be added to it? I don't see much value in this file as is.
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.
Originally config yml was assumed to include list of rules and their individual configurations, which brings possibilities of large ymls with customizations depending on use cases of the evaluator. We don't have clear picture of what it will be. Dylan's suggestion is to make it simplest at this phase. I agree with you that it doesn't have much value as it is.
dataset.push resource | ||
end | ||
|
||
puts "Loaded #{dataset.length} resources" |
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.
We shouldn't have print statements outside of the CLI.
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.
Removed all print statements. Will come back when we have logger for the evaluator.
module Inferno | ||
module DSL | ||
module FHIREvaluation | ||
# A Dataset returns an Array of FHIR data to be summarized or evaluated, |
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.
This comment is out of date.
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.
Removed
class EvaluationContext | ||
attr_reader :ig, :data, :results, :config | ||
|
||
def initialize(the_ig, data, config) |
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.
I don't see why the_ig
isn't just named ig
.
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.
Modified
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.
It's because there's a rubocop complaint if we use just ig
unfortunately:
lib/inferno/dsl/fhir_evaluator/evaluation_context.rb:12:24:
C: Naming/MethodParameterName: Method parameter must be at least 3 characters long.
def initialize(ig, data, config)
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.
Oh, right. It doesn't pass Rubocop with ig
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.
This is definitely a case where you should disable that rule for this line.
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.
Done
@@ -0,0 +1,44 @@ | |||
# frozen_string_literal: true | |||
|
|||
require 'dotenv' |
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.
I don't know that any of these requires are necessary.
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.
Removed
@@ -0,0 +1,21 @@ | |||
module Inferno | |||
module DSL | |||
module FHIREvaluation |
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.
The module name and the file path don't match.
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.
Addressed
class EvaluationContext | ||
attr_reader :ig, :data, :results, :config | ||
|
||
# rubocop:disable Naming/MethodParameterName |
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.
Instead of using a disable and an enable, just put the disable at the end of the relevant line, and then it will only affect that line and you don't have to reenable the rule.
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.
Thanks for the tips. Modified.
Summary
Since the evaluator requires a validator service and fhirpath service, it seems to make the most sense to have the evaluator be part of inferno core. Migrate the CLI class into inferno core such that it can be invoked with
bundle exec inferno evaluate ....
.This does not include any other integrations at this point - the IG does not need to be parsed, no rules need to run against the data, etc. However this may include or touch additional files needed to require everything, etc
Method
/dsl
/dsl/fhir_evaluator
Rule files are not added at this point to pass Rubocop.evaluate
methodAll test cases passed and rubocop passed
Testing Guidance
To test, run command:
bundle exec inferno evaluate
Nothing actually happens but a message of execution confirmation.