Merge pull request #900 from wtsi-npg/devel

pull from devel to master to create release 74.0.1
wtsi-npg · Dec 17, 2024 · 3d77241 · 3d77241
2 parents f9f614f + cf588d5
commit 3d77241
Show file tree

Hide file tree

Showing 7 changed files with 160 additions and 77 deletions.
diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml
@@ -34,7 +34,8 @@ jobs:
           sudo apt-get update
           # https://github.com/actions/runner-images/issues/2139
           sudo apt-get remove -y nginx libgd3
-          sudo apt-get install -y libgd-dev uuid-dev libgd-text-perl
+          # libdevel-patchperl-perl is needed for perlbrew
+          sudo apt-get install -y libgd-dev uuid-dev libgd-text-perl libdevel-patchperl-perl
 
       - name: "Install Miniforge"
         run: |

diff --git a/Changes b/Changes
@@ -1,5 +1,15 @@
 LIST OF CHANGES FOR NPG-QC PACKAGE
 
+release 74.0.1 (2024-12-17)
+ - Fix perlbrew installation by installing libdevel-patchperl-perl in runner
+ - Fixes/updates for lane-level RoboQC lane-level assessment:
+    - Excluded study-specific lane-lane assessment in npg_qc::autoqc::check::review
+      This helps to avoid uncertainty in evaluating lanes where samples belong to
+      different studies at a price of not being able to perform study-specific
+      evaluation of one-library lanes.
+    - Updated the expression evaluator to allow for multiple types of
+      result objects to be used in a single expression.
+
 release 74.0.0 (2024-12-02)
  - npg_qc::autoqc::qc_store - changed loading of ORM classes into memory
    to on demand at run time.

diff --git a/lib/npg_qc/autoqc/checks/review.pm b/lib/npg_qc/autoqc/checks/review.pm
@@ -24,25 +24,36 @@ our $VERSION = '0';
 Readonly::Scalar my $CONJUNCTION_OP => q[and];
 Readonly::Scalar my $DISJUNCTION_OP => q[or];
 
-Readonly::Scalar my $ROBO_KEY         => q[robo_qc];
-Readonly::Scalar my $CRITERIA_KEY     => q[criteria];
-Readonly::Scalar my $QC_TYPE_KEY      => q[qc_type];
+Readonly::Scalar my $ROBO_KEY => q[robo_qc];
+Readonly::Scalar my $CRITERIA_KEY => q[criteria];
 Readonly::Scalar my $APPLICABILITY_CRITERIA_KEY => q[applicability_criteria];
 Readonly::Scalar my $LIMS_APPLICABILITY_CRITERIA_KEY => q[lims];
 Readonly::Scalar my $SEQ_APPLICABILITY_CRITERIA_KEY => q[sequencing_run];
 Readonly::Array  my @APPLICABILITY_CRITERIA_TYPES => (
   $LIMS_APPLICABILITY_CRITERIA_KEY, $SEQ_APPLICABILITY_CRITERIA_KEY
                                                      );
-Readonly::Scalar my $ACCEPTANCE_CRITERIA_KEY    => q[acceptance_criteria];
+Readonly::Scalar my $ACCEPTANCE_CRITERIA_KEY => q[acceptance_criteria];
 
 Readonly::Scalar my $QC_TYPE_LIB => q[mqc];
 Readonly::Scalar my $QC_TYPE_SEQ => q[mqc_seq];
 
 Readonly::Scalar my $TIMESTAMP_FORMAT_WOFFSET => q[%Y-%m-%dT%T%z];
 
 Readonly::Scalar my $CLASS_NAME_SPEC_DELIM => q[:];
-Readonly::Scalar my $CLASS_NAME_SPEC_RE    =>
-  qr/\A(?:\W+)? (\w+) (?: $CLASS_NAME_SPEC_DELIM (\w+))?[.]/xms;
+Readonly::Scalar my $CLASS_NAME_SPEC_RE =>
+  qr{
+    # Examples:
+    # "generic:ncov2019_artic_nf.doc->{meta}->{'num_input_reads'} > 10000"
+    # "tag_metrics.all_reads && (qX_yield.yield1_q30/(tag_metrics.all_reads * 302) > 78)"
+
+    (?:\W+)? # optional non-word characters
+    ( # start of capture group
+    [[:alpha:]_]+ # result class name
+    (?: $CLASS_NAME_SPEC_DELIM \w+)? # an optional spec for a generic result
+                                     # class prepended by a delimeter
+    ) # end of capture group
+    [.]  # ends with a dot
+  }xms;
 
 ## no critic (Documentation::RequirePodAtEnd)
 
@@ -74,7 +85,8 @@ outcomes.
 The C<robo> section of the product configuration file sits either within
 the configuration for a particular study or in the C<default> section, or
 in both locations. For a given entity a study-specific RoboQC definition
-takes precedence over the default one.
+takes precedence over the default one. For lanes only the C<default> section
+of the product configuration file is examined.
 
 Evaluation criteria for samples vary depending on the sequencing instrument
 and flowcell type, library type, sample reference, etc. Each of the
@@ -487,14 +499,24 @@ has '_robo_config' => (
 sub _build__robo_config {
   my $self = shift;
 
-  my $strict = 1; # Parse study section only, ignore the default section.
-  my $config = $self->study_config($self->lims(), $strict);
-  if ($config) {
-    $config = $config->{$ROBO_KEY};
+  my $config;
+
+  # Do not examine study-specific section for lane entities, meaning that
+  # one-sample lanes, pools or not, cannot be evaluated according to the
+  # study criteria of their sample. Helps to avoid uncertainty of dealing
+  # with pool's samples belonging to different studies.
+  if (!$self->lims->is_lane) {
+    my $strict = 1; # Parse study section only, ignore the 'default' section.
+    $config = $self->study_config($self->lims(), $strict);
+    if ($config) {
+      $config = $config->{$ROBO_KEY};
+    }
+    if (!$config) {
+      carp 'Study-specific RoboQC config not found for ' . $self->_entity_desc;
+    }
   }
 
-  if (!$config) {
-    carp 'Study-specific RoboQC config not found for ' . $self->_entity_desc;
+  if (!$config) { # Look at the 'default' section of the configuration file.
     $config = $self->default_study_config()->{$ROBO_KEY};
   }
 
@@ -652,7 +674,7 @@ sub _build__result_class_names {
   my $self = shift;
 
   my @class_names = uniq sort
-                    map { (_class_name_from_expression($_))[0] }
+                    map { _class_names_from_expression($_) }
                     map { @{$_} } # dereference the array
                     values %{$self->_criteria()};
 
@@ -721,11 +743,26 @@ sub _build__results {
   return \%h;
 }
 
-sub _class_name_from_expression {
+sub _class_names_with_spec_from_expression {
+  my $e = shift;
+  # Note 'g' flag at the end of the regexp. We will get a list of matches.
+  # The list might contain multiple class names for the same class. Depending
+  # on the expression, the return value contains one or more class names.
+  my @extended_class_names = $e =~ /$CLASS_NAME_SPEC_RE/xmsg;
+  @extended_class_names = uniq @extended_class_names; # Remove repetitions.
+  @extended_class_names or croak "Failed to infer class names from $e";
+  return @extended_class_names;
+}
+
+sub _class_names_from_expression {
   my $e = shift;
-  my ($class_name, $spec) = $e =~ $CLASS_NAME_SPEC_RE;
-  $class_name or croak "Failed to infer class name from $e";
-  return ($class_name, $spec);
+  my @names = ();
+  foreach my $name (_class_names_with_spec_from_expression($e)) {
+    ($name) = $name =~ /\A(\w+)/smx; # Remove the spec part if present.
+    push @names, $name;
+  }
+  @names = uniq @names;
+  return @names;
 }
 
 #####
@@ -785,62 +822,68 @@ sub _apply_operator {
   return $outcome ? 1 : 0;
 }
 
+sub _pp_name2spec {
+  # To get a match with what would have been used in the robo config,
+  # replace all 'non-word' characters.
+  my $pp_name = shift;
+  $pp_name =~ s/\W/_/gsmx;
+  return $pp_name;
+};
+
+sub _get_evaluation_result {
+  my ($perl_e, $e, $results) = @_;
+
+  use warnings FATAL => 'uninitialized'; # Force an error when operations on
+                                         # undefined values are attempted.
+  ##no critic (BuiltinFunctions::ProhibitStringyEval)
+  my $o = eval $perl_e; # Evaluate Perl expression
+  ##use critic
+  if ($EVAL_ERROR) {
+    my $err = $EVAL_ERROR;
+    croak "Error evaluating expression '$perl_e' derived from '$e': $err";
+  }
+
+  return $o ? 1 : 0;
+}
+
 #####
 # Evaluates a single expression in the context of available autoqc results.
 # If runs successfully, returns 0 or 1, otherwise throws an error.
 #
 sub _evaluate_expression {
   my ($self, $e) = @_;
 
-  my ($class_name, $spec) = _class_name_from_expression($e);
-  my $obj_a = $self->_results->{$class_name};
-  # We should not get this far with an error in the configuration
-  # file, but just in case...
-  $obj_a and @{$obj_a} or croak "No autoqc result for evaluation of '$e'";
-
-  if ($spec) {
-    my $pp_name2spec = sub {    # To get a match with what would have been
-      my $pp_name = shift;      # used in the robo config, replace all
-      $pp_name =~ s/\W/_/gsmx;  # 'non-word' characters.
-      return $pp_name;
-    };
-    # Now have to choose one result. If the object is not an instance of
-    # the generic autoqc result class, there will be an error at this point.
-    # Making the code less specific is not worth the effort at this point.
-    my @on_spec = grep { $pp_name2spec->($_->pp_name) eq $spec } @{$obj_a};
-    @on_spec or croak "No autoqc $class_name result for $spec";
-    $obj_a = \@on_spec;
-  }
-
-  (@{$obj_a} == 1) or croak "Multiple autoqc results for evaluation of '$e'";
-  my $obj = $obj_a->[0];
-
-  $obj or croak "No autoqc result for evaluation of '$e'";
-
-  # Prepare the expression from the robo config for evaluation.
-  my $placeholder = $class_name;
-  $spec and ($placeholder .= $CLASS_NAME_SPEC_DELIM . $spec);
-  my $replacement = q[$] . q[result->];
+  my @names = _class_names_with_spec_from_expression($e);
+  my $results = {};
   my $perl_e = $e;
-  $perl_e =~ s/$placeholder[.]/$replacement/xmsg;
-
-  my $evaluator = sub { # Evaluation function
-    my $result = shift;
-    # Force an error when operations on undefined values are
-    # are attempted.
-    use warnings FATAL => 'uninitialized';
-    ##no critic (BuiltinFunctions::ProhibitStringyEval)
-    my $o = eval $perl_e; # Evaluate Perl string expression
-    ##use critic
-    if ($EVAL_ERROR) {
-      my $err = $EVAL_ERROR;
-      croak "Error evaluating expression '$perl_e' derived from '$e': $err";
+  # Prepare the expression from the robo config for evaluation.
+  foreach my $name (@names) {
+    my ($class_name, $spec) = split /$CLASS_NAME_SPEC_DELIM/smx, $name;
+    my $obj_a = $self->_results->{$class_name};
+    # We should not get this far with an error in the configuration
+    # file, but just in case...
+    $obj_a and @{$obj_a} or croak "No $class_name autoqc result for evaluation of '$e'";
+    if ($spec) {
+      # Now have to choose one result. If the object is not an instance of
+      # the generic autoqc result class, there will be an error at this point.
+      # Making the code less specific is not worth the effort at this point.
+      my @on_spec = grep { _pp_name2spec($_->pp_name) eq $spec } @{$obj_a};
+      @on_spec or croak "No $name autoqc result for evaluation of '$e'";
+      $obj_a = \@on_spec;
     }
-    return $o ? 1 : 0;
-  };
+    (@{$obj_a} == 1) or croak "Multiple $name autoqc results for evaluation of '$e'";
+    my $obj = $obj_a->[0];
+    $obj or croak "No $class_name autoqc result for evaluation of '$e'";
+    $results->{$name} = $obj;
+
+    ## no critic (ValuesAndExpressions::RequireInterpolationOfMetachars)
+    my $replacement = q[$results->{'] . $name . q['}->];
+    ## use critic
+    $perl_e =~ s/$name[.]/$replacement/xmsg;
+  }
 
   # Evaluate and return the outcome.
-  return $evaluator->($obj);
+  return _get_evaluation_result($perl_e, $e, $results);
 }
 
 __PACKAGE__->meta->make_immutable();

diff --git a/scripts/install_miniforge.sh b/scripts/install_miniforge.sh
@@ -8,8 +8,8 @@
 
 set -ex
 
-MINIFORGE_VERSION="24.9.0-0"
-MINIFORGE_SHA256="77fb505f6266ffa1b5d59604cf6ba25948627e908928cbff148813957b1c28af"
+MINIFORGE_VERSION="24.9.2-0"
+MINIFORGE_SHA256="ca8c544254c40ae5192eb7db4e133ff4eb9f942a1fec737dba8205ac3f626322"
 
 CONDA_HOME=${CONDA_HOME:="$HOME/conda"}
 export CONDA_HOME
@@ -27,7 +27,7 @@ channels:
   - conda-forge
 EOF
 
-curl -sSL "https://github.com/conda-forge/miniforge/releases/download/${MINIFORGE_VERSION}/Mambaforge-${MINIFORGE_VERSION}-Linux-x86_64.sh" -o ./miniforge.sh
+curl -sSL "https://github.com/conda-forge/miniforge/releases/download/${MINIFORGE_VERSION}/Miniforge3-${MINIFORGE_VERSION}-Linux-x86_64.sh" -o ./miniforge.sh
 sha256sum ./miniforge.sh | grep "$MINIFORGE_SHA256"
 /bin/bash ./miniforge.sh -b -p "$CONDA_HOME"
 rm ./miniforge.sh