Skip to content

Run check_config on methods too - #37

Open
rcannood wants to merge 1 commit into
mainfrom
fix/enable-check-config-for-methods
Open

Run check_config on methods too#37
rcannood wants to merge 1 commit into
mainfrom
fix/enable-check-config-for-methods

Conversation

@rcannood

Copy link
Copy Markdown
Member

Describe your changes

comp_control_method.yaml and comp_metric.yaml both list check_config.py in their test resources. comp_method.yaml doesn't -- so method metadata has never been validated. That's how four configs kept the deprecated placement:

info:
  documentation_url: https://cran.r-project.org/package=FNN
  repository_url: https://github.com/cran/FNN

when check_config() requires links.documentation and links.repository at the top level for type: method. Moved them in knnr_py, knnr_r, lm and guanlab_dengkw_pm.

Turning the check on also surfaced something pre-existing: novel and simple_mlp have a Nextflow runner with no directives, which trips

AssertionError: label not a field in nextflow runner directives

simple_mlp was already failing this on main -- it merges wf_method.yaml, which has always had check_config.py. It just never ran, because CI only tests components touched by a PR and nobody has touched simple_mlp in a while. Gave both wrappers a [lowtime, lowmem, lowcpu] label with a comment noting the real resources live on the train/predict sub-components.

Ran check_config() over every component after the change:

  control_methods/mean_per_gene       PASS
  control_methods/random_predict      PASS
  control_methods/solution            PASS
  control_methods/zeros               PASS
  methods/cellmapper_linear           PASS
  methods/cellmapper_scvi             PASS
  methods/guanlab_dengkw_pm           PASS
  methods/knnr_py                     PASS
  methods/knnr_r                      PASS
  methods/lm                          PASS
  methods/novel                       PASS
  methods/simple_mlp                  PASS
  metrics/correlation                 PASS
  metrics/mse                         PASS

Heads-up for the open method PRs (#16, #18, #19, #20): once this lands they'll need links.documentation and links.repository too. That's the point of the check, but worth flagging so it isn't a surprise.

Part of a series of PRs coming out of a pre-run review of the benchmark.

Checklist before requesting a review

  • I have performed a self-review of my code

  • Check the correct box. Does this PR contain:

    • Breaking changes
    • New functionality
    • Major changes
    • Minor changes
    • Bug fixes
  • Proposed changes are described in the CHANGELOG.md

  • CI Tests succeed and look good!

* Add check_config.py to comp_method.yaml's test resources
* Move documentation_url/repository_url to links in the four configs
  that still used the old placement
* Give novel and simple_mlp a nextflow resource label

Control methods and metrics were checked, methods were not, which is how
the deprecated info.documentation_url survived in four configs.
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.

1 participant