met.process: ensure host arg is passed on as a list

[infotroph]

Description

The immediate bug, reported by @AritraDey-Dev in Slack: When passing a whole settings$host block in to met.process, R >=4.3 fails because we put a condition with length >1 into a scalar || operator. Fixed by refactoring to only look at the element we care about.

Potential issue addressed while I was here: When host isn't specified, met.process defaults to the bare string "localhost", which is fine in this file but is then passed along to some downstream functions that are documented to expect a named list. Fixed by wrapping bare strings as host <- list(name = host) before passing it along.

Motivation and Context

Review Time Estimate

• Immediately
• Within one week
• When possible

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My change requires a change to the documentation.
• My name is in the list of CITATION.cff
• I agree that PEcAn Project may distribute my contribution under any or all of
  • the same license as the existing code,
  • and/or the BSD 3-clause license.
• I have updated the CHANGELOG.md.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[AritraDey-Dev]

Thanks for looking into this. However, after applying both of the changes suggested by @infotroph and @Sweetdevil144, the issue still persists.

few Observations from my side ---
If you check the Rmd template I am currently working on (#3486), I encountered the same issue while running PEcAn.workflow::do_conversions.
I have attached some images below to illustrate the error.

While executing:

settings <- PEcAn.workflow::do_conversions(settings)

I encountered the same issue again. Upon logging str(settings$host), it displayed this output (see the attached image).

Temporary Fix in RStudio

To resolve this in RStudio, I applied the following fix:

if (!is.list(settings$host)) {
  settings$host <- list(name = settings$host)
}

So I think adding this if statement will fix it.
if (!is.list(host)) { host <- list(name = host) }

@infotroph any insights on this will be helpful.

[infotroph]

@AritraDey-Dev Thanks for testing! I see that ic_process and soil_process, both of which can be called by do.conversions, contained copy-pasted versions of the same code, and have now pushed fixes for those too.

But! Processing of soil and IC ("initial conditions") data don't usually happen in simple workflows -- they require a fair amount of extra configuration. If you don't already know you're using one of those, I would instead check whether your test run really used the patched version of met.process.

[infotroph]

Oh, and regarding is.list: Which file did you add it to? The !hasName on line 96 of met.process ought to handle the list case, so if it doesn't then we should revisit. (I used hasName instead of is.list to avoid needing to care whether host is a list or a named vector when the thing we actually care about is "Will it be an error to write host$name later?").

[AritraDey-Dev]

@AritraDey-Dev Thanks for testing! I see that ic_process and soil_process, both of which can be called by do.conversions, contained copy-pasted versions of the same code, and have now pushed fixes for those too.

But! Processing of soil and IC ("initial conditions") data don't usually happen in simple workflows -- they require a fair amount of extra configuration. If you don't already know you're using one of those, I would instead check whether your test run really used the patched version of met.process.

As there were multiple instances, I already changed it in both places. that doesn't work too.
Since you mentioned that a simple workflow doesn’t require it, it also makes sense that changing it in both places didn’t fix the issue either.

[AritraDey-Dev]

Oh, and regarding is.list: Which file did you add it to? The !hasName on line 96 of met.process ought to handle the list case, so if it doesn't then we should revisit. (I used hasName instead of is.list to avoid needing to care whether host is a list or a named vector when the thing we actually care about is "Will it be an error to write host$name later?").

it didn't fix the issue for running workflow in web...but while working on this PR #3486 it did fix it(for a rmd template).
But i am pretty sure this is causing because of while PEcAn.workflow::do_conversions.

[Sweetdevil144]

Suggested change

	if (length(host) == 1 && !hasName(host, "name")) {
	if (length(host) == 1 && !("name" %in% names(host))) {

Would help bypass the make check errors without changing the NAMESPACE