-
Notifications
You must be signed in to change notification settings - Fork 282
Fix regression in convert_input.R and check_missing_files.R #3724
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
Fix regression in convert_input.R and check_missing_files.R #3724
Conversation
Signed-off-by: Aritra Dey <adey01027@gmail.com>
|
Just to add, after digging into the git history, this issue seems to have been introduced in PR #3338. |
mdietze
left a 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.
I'm fine with sorting out the requested fixes, but ultimately they shouldn't be causing the issues you are discussing -- the meta-analysis code should not depend on the met data from convert.inputs in any way.
| # Get machine information | ||
| machine.info <- get_machine_info(host, input.args = input.args, input.id = input.id, con = con) | ||
|
|
||
| if (any(sapply(machine.info, is.null))) { |
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.
Not sure about this change. If there's only 1 machine info it seems like the old version should still work, but if there's more than 1 the new version will definitely fail
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.
get_machine_info returns a single list with three named elements: list(machine = ..., input = ..., dbfile = ...). It does not return a list of multiple machine infos.
For a new input (which is the case failing here), input.id is NULL. In this scenario, get_machine_info explicitly sets input and dbfile to NULL and returns:
list(machine = <data.frame>, input = NULL, dbfile = NULL)
This is a valid state for a new file download.
However, the old check any(sapply(machine.info, is.null)) iterates over these three elements. Since input and dbfile are NULL, it evaluates to TRUE, incorrectly flagging this valid state as a fatal error and stopping the workflow.
The new check is.null(machine.info) is correct because get_machine_info returns NULL (the whole object) only when the machine lookup itself fails, which is the actual error condition we want to catch.
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 old check also behaved badly in the single null case because sapply(NULL, is.null) returns an empty list and then any(list()) returns FALSE.
infotroph
left a 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.
Thanks for tracking this down!
Yes i agree it is not a meta analysis issue,the thing is |
3b6cc67
|
@robkooper should we pull this into release too? |
Description
Running a workflow that has a db connection and attempts to download met data is failing silently after the meta-analysis step with the following error:
The same issue occurs in the Docker stack tests.
This PR fixes a regression in
convert_input.Rwhere met file downloads were silently skipped, causing downstream sample.int errors inensemble.Rwhen the workflow fell back to missing met data after meta-analysis. The fix relaxes an incorrect NULL check and ensurescheck_missing_filesreturns a named list.Motivation and Context
Review Time Estimate
Types of changes
Checklist: