Skip to content

Give file_pretrained_model an example - #52

Closed
rcannood wants to merge 1 commit into
mainfrom
fix/pretrained-model-example
Closed

Give file_pretrained_model an example#52
rcannood wants to merge 1 commit into
mainfrom
fix/pretrained-model-example

Conversation

@rcannood

Copy link
Copy Markdown
Member

Describe your changes

novel_train and simple_mlp_train cannot pass viash test on main, and haven't been able to for a while:

+/tmp/viash_test_novel_train_.../test_run_and_check_output/test_executable ---cpus=2 ---memory=14gb
[error] --output is a required argument. Use --help to get more information on the parameters.
AssertionError: Script exited with an error. Return code: 1

get_argument_sets() only gives an argument a value when the spec has a default or an example:

if arg.get("default") is not None:
    default_or_example = arg["default"]
elif arg.get("example") is not None:
    default_or_example = arg["example"]
...
if default_or_example is not None and arg["type"] == "file":

file_pretrained_model.yaml was the only file spec with neither, so --output never made it onto the command line:

  --input_train_mod1  type=file  dir=input   example=[...train_mod1.h5ad]
  --input_train_mod2  type=file  dir=input   example=[...train_mod2.h5ad]
  --input_test_mod1   type=file  dir=input   example=[...test_mod1.h5ad]
  --output            type=file  dir=output  example=None   <-- here

With an example added:

  novel_train          [run] --output passed: True
  simple_mlp_train     [run] --output passed: True
  novel_predict        [with_model] --output passed: True
  simple_mlp_predict   [with_model] --output passed: True

For an output argument only the extension is read off the example, so a directory-shaped path gives the right thing (the pretrained model is a directory). The predict components already override --input_model through info.test_setup, so they're unaffected either way.

Worth knowing: this is why #46 is red, and #33 was red for the same reason before it was merged. CI only tests components a PR touches, so it only shows up when someone edits novel_train or simple_mlp_train -- and then it looks like that PR broke something.

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!

run_and_check_output only assigns a value to an argument when the file
spec has a default or an example, so --output was silently left off the
command line and novel_train/simple_mlp_train failed on a missing
required argument.
@rcannood

Copy link
Copy Markdown
Member Author

these examples should be set in the predict methods under info.test_setup.with_model.input_model because they are different for each method.

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